-
Notifications
You must be signed in to change notification settings - Fork 233
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
Added "-y" Option to convert command to directly convert the config #621
Conversation
Codecov Report
@@ Coverage Diff @@
## master #621 +/- ##
==========================================
+ Coverage 77.16% 78.01% +0.85%
==========================================
Files 6 6
Lines 832 828 -4
Branches 239 239
==========================================
+ Hits 642 646 +4
+ Misses 131 122 -9
- Partials 59 60 +1
Continue to review full report at Codecov.
|
Do You mean just a rebase that sqaushes the 4 commits into one or a rebase that is in sync with the current state of the master branch? |
@aRkedos Hi there! Rebase in sync with the current master branch |
cda1d2d
to
c9c2910
Compare
Okay rebased it :) Would like to work on other issues / freature requests too, but don't want to do too much at the same time; so I'll wait for your feedback. |
Nice @aRkedos. I made 2 contributions to the function, what do you think?
|
Ah that totally makes sense and removes the redundancies in the if branches. My Idea was to write a helper function which does the actual file writing which is called after confirmation (be it via prompt or via option). But this way is much clearer. And I see why my tests which I wrote initially did not affect the code coverage (was my inexpierience with pytest), I was using the invoke method so my additional branches were not covered by the test since they were not executed. I will update the changelog and document the behaviour. |
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 to me! Thank you @aRkedos !
I implemented the feature requested in #589. I ran all test except the one described in issue #620.
I extensively tested the feature with a a lot of configs. I went with "-y" like requested in #589 but maybe it would be more concise to do
-f
/--force
like PR #618 does with the freeze command.