-
Notifications
You must be signed in to change notification settings - Fork 104
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
Add "elsewhere" option to fetch, pull and push menu #160
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #160 +/- ##
==========================================
+ Coverage 86.33% 86.51% +0.18%
==========================================
Files 56 58 +2
Lines 4413 4473 +60
==========================================
+ Hits 3810 3870 +60
Misses 603 603 ☔ View full report in Codecov by Sentry. |
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.
Fantastic! :)
I had some minor things just. Tested it out and seems to work!
Tests
A remote is actually set up to a temp dir when running tests, so can easily be tested!
They'd typically snapshot both the prompt and the end result.
Just to see that the command was executed is enough IMO.
#[test]
fn push_elsewhere_prompt() {
snapshot!(TestContext::setup_clone(), "Pe");
}
#[test]
fn push_elsewhere() {
snapshot!(TestContext::setup_clone(), "Peorigin<enter>");
}
See src/tests/push.rs
or src/tests/mod.rs
Changelog
It would be nice with a changelog entry.
if You prefix your first commit with "feat: ", this will be generated automatically.
Refer to git cliff --unreleased
, make test
or cliff.toml
We should be ready for another round of review |
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.
Looks good!
CHANGELOG.md
Outdated
|
||
- Add "elsewhere" option to fetch, pull and push menu | ||
- Syntax highlighting for Elixir | ||
|
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.
Ah, I should've noted in my suggestion: this is generated when I make a release. So the commit Add changelog entry wouldn't be needed.
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'll squash the commits and merge it to master.
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.
oh, I already force-pushed to remove the commit
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.
👍 Ah you're fast! My merge failed because of it. Let's include all the commits then :)
Implements #139.