-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add worktree feature with tests #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@racgoo is attempting to deploy a commit to the Toss Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
src/worktree.rs
Outdated
| /// @param {Repository} repo - Repository to open worktree from. | ||
| /// @returns Worktree instance. | ||
| /// @throws Throws error if the repository is not a worktree or if opening fails. | ||
| pub fn open_from_repository(repo: Reference<Repository>, env: Env) -> crate::Result<Worktree> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this method as function?
Other constructor methods (e.g. openRepository, initRepository also use functions instead of static methods, so it would be good to unify interfaces.
Also, is colud be better if it were async.
e.g.
#[napi]
/// @signature
/// ```ts
/// function openWorktreeFromRepository(
/// repo: Repository,
/// ): Promise<Worktree>;
/// ```
pub fn open_worktree_from_repository(...) -> AsyncTask<WorktreeOpenFromRepositoryTask> {
todo!()
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seokju-na Sorry for the late reply. spent some time thinking through the async design here.
My current thinking is to use AsyncTask only when the operation can be
expressed in terms of pure Rust values (e.g. String, PathBuf, primitive
types) that can safely be moved to a worker thread.
In cases like this one, a real AsyncTask is not possible because
napi-managed objects cannot be moved to worker threads.
One alternative is a fake-async Promise-based API (sync execution +
immediate resolve), which would require upgrading napi-rs to >= 3.7.1.
For reference, the implementation would look like this
pub fn open_worktree_from_repository(repo: &Repository, env: Env) -> crate::Result<PromiseRaw<'_,Worktree>> {
let git2_worktree = git2::Worktree::open_from_repository(&repo.inner);
match git2_worktree {
Ok(worktree) => {
Ok(PromiseRaw::resolve(&env, Worktree { inner: worktree })?)
},
Err(error) => {
Ok(PromiseRaw::reject(&env, error.to_string())?)
}
}
}Does this approach sound reasonable to you?
Oh.. it's mistake Co-authored-by: seokju-na <seokju.me@gmail.com>
I used '.map' here for brevity, even though it's not ideal for mutable side effects. Thanks for the suggestion. I'll refactor this to an if let pattern. Co-authored-by: seokju-na <seokju.me@gmail.com>
Same here. I’ll switch this to 'if let' for clarity. Thanks! Co-authored-by: seokju-na <seokju.me@gmail.com>
Summary
Changes
Closes #172