feat: add reflog feature with tests #180
Conversation
|
@racgoo is attempting to deploy a commit to the Toss Team on Vercel. A member of the Team first needs to authorize it. |
src/reflog.rs
Outdated
| /// ``` | ||
| /// | ||
| /// @returns Message bytes of this reflog entry. Returns `null` if no message is present. | ||
| pub fn message_bytes(&self) -> Option<Uint8Array> { |
There was a problem hiding this comment.
I think this method seems unnecessary, as it's rare case to handle messages as a uint8array.
Instead of returning a nullable message in message() method, how about throw an error if the message isn't UTF-8?
e.g.
pub fn message(&self) -> crate::Result<Option<String>> {
// ... throw if message is not utf-8
}There was a problem hiding this comment.
I agree. Rather than fully exposing git2’s interface, I’ll adopt the more practical approach you suggested.
src/reflog.rs
Outdated
| /// @param {number} i - Index of the entry to remove. | ||
| /// @param {boolean} rewritePreviousEntry - Whether to rewrite the previous entry. | ||
| /// @throws Throws error if the index is invalid or if removal fails. | ||
| pub fn remove(&mut self, i: u32, rewrite_previous_entry: bool) -> crate::Result<()> { |
There was a problem hiding this comment.
| pub fn remove(&mut self, i: u32, rewrite_previous_entry: bool) -> crate::Result<()> { | |
| pub fn remove(&mut self, i: u32, rewrite_previous_entry: Option<bool>) -> crate::Result<()> { |
Since libgit2 expect rewrite_previous_entry default value as 0, so how about receiving it as an optional value and handling the default value internally?
There was a problem hiding this comment.
I missed that. I’ll change it to “Option” and handle the default internally to match git2’s behavior. Thanks!
|
About the question: Since |
|
@seokju-na I’ve addressed all the review comments and pushed an updated commit. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
2 participants
Summary
Changes
Question
@seokju-na I have a small question regarding the error-handling pattern used here (also seen in 'tree.rs' and 'stash.rs').
In 'reflog.get' (reflog.rs:228), the error created by 'ok_or(Error::new(...))' seems to be discarded immediately because the result is converted with '.ok()', collapsing all errors into 'None'.
Since I couldn't find a better approach myself, I wanted to ask whether there is a more idiomatic way to express this without constructing an unused error.
Closes #171