-
Notifications
You must be signed in to change notification settings - Fork 504
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
Apply from local branches #4342
Conversation
e4c0f3f
to
7dbab47
Compare
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.
I think I got quite stuck on the function that deals with local tracking branches, apologies for all the comments and ideas that sparked.
.transpose(), | ||
Err(_) => Ok(None), | ||
} | ||
let Ok(name) = name else { return Ok(None) }; |
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.
This could be less repetitive/more concise.
let Ok(name) = name else { return Ok(None) }; | |
let (Ok(name), Some(sha)) = (name, branch.get().target()) else { return Ok(None) }; |
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.
I've seen that sort of pattern elsewhere and I'm not sure if I like it. To me it looks quite cluttered having it all on one line.
If it is the preferred way of doing it though I guess I should get used it it
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.
The line isn't formatted, so I think it will look better after formatting.
But even then, if you don't like it, that should be enough to keep things as is.
The majority of the work here has already been superseded by other PRs, or merged in separately. |
☕️ Reasoning
🧢 Changes