-
Notifications
You must be signed in to change notification settings - Fork 122
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 --keep option to allow to generate newsfile, but keep newsfragmen… #453
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
19f1fe0
Add --keep option to allow to generate newsfile, but keep newsfragmen…
fizyk 0ee66f1
Apply suggestions from code review
fizyk 3b44cec
Additional post-PR enhancements
fizyk 6dc3e63
Update build --keep comment to be more relevant
fizyk bf049f7
More compact remove_files and use with_isolated_runner for new tests
fizyk 0445078
Refactored _git.remove_files
fizyk 1908de0
Update src/towncrier/newsfragments/129.feature
fizyk ab17b50
Make a decision and return it instead of calling _git.remove underneath
fizyk c5dadf0
Move should_remove_fragment_files into build module
fizyk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
# Copyright (c) Amber Brown, 2015 | ||
# See LICENSE for details. | ||
|
||
from __future__ import annotations | ||
|
||
import click | ||
|
||
from towncrier._git import remove_files | ||
|
||
|
||
def remove_news_fragment_files( | ||
fragment_filenames: list[str], answer_yes: bool, answer_keep: bool | ||
) -> None: | ||
try: | ||
if answer_keep: | ||
click.echo("Keeping the following files:") | ||
# Not proceeding with the removal of the files. | ||
return | ||
|
||
if answer_yes: | ||
click.echo("Removing the following files:") | ||
else: | ||
click.echo("I want to remove the following files:") | ||
finally: | ||
# Will always be printed, even for answer_keep to help with possible troubleshooting | ||
for filename in fragment_filenames: | ||
click.echo(filename) | ||
|
||
if answer_yes or click.confirm("Is it okay if I remove those files?", default=True): | ||
remove_files(fragment_filenames) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Added `--keep` option to build command that allows to generate newsfile but keep newsfragments in place. This option can not be used together with `--yes`. | ||
fizyk marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -407,6 +407,66 @@ def test_no_confirmation(self): | |
self.assertFalse(os.path.isfile(fragment_path1)) | ||
self.assertFalse(os.path.isfile(fragment_path2)) | ||
|
||
@with_isolated_runner | ||
def test_keep_fragments(self, runner): | ||
""" | ||
The `--keep` option will build the full final news file | ||
without deleting the fragment files and without | ||
any extra CLI interaction or confirmation. | ||
""" | ||
setup_simple_project() | ||
fragment_path1 = "foo/newsfragments/123.feature" | ||
fragment_path2 = "foo/newsfragments/124.feature.rst" | ||
with open(fragment_path1, "w") as f: | ||
f.write("Adds levitation") | ||
with open(fragment_path2, "w") as f: | ||
f.write("Extends levitation") | ||
|
||
call(["git", "init"]) | ||
call(["git", "config", "user.name", "user"]) | ||
call(["git", "config", "user.email", "[email protected]"]) | ||
call(["git", "add", "."]) | ||
call(["git", "commit", "-m", "Initial Commit"]) | ||
|
||
result = runner.invoke(_main, ["--date", "01-01-2001", "--keep"]) | ||
|
||
self.assertEqual(0, result.exit_code) | ||
# The NEWS file is created. | ||
# So this is not just `--draft`. | ||
self.assertTrue(os.path.isfile("NEWS.rst")) | ||
self.assertTrue(os.path.isfile(fragment_path1)) | ||
self.assertTrue(os.path.isfile(fragment_path2)) | ||
|
||
@with_isolated_runner | ||
def test_yes_keep_error(self, runner): | ||
""" | ||
It will fail to perform any action when the | ||
conflicting --keep and --yes options are provided. | ||
|
||
Called twice with the different order of --keep and --yes options | ||
to make sure both orders are validated since click triggers the validator | ||
in the order it parses the command line. | ||
""" | ||
setup_simple_project() | ||
fragment_path1 = "foo/newsfragments/123.feature" | ||
fragment_path2 = "foo/newsfragments/124.feature.rst" | ||
with open(fragment_path1, "w") as f: | ||
f.write("Adds levitation") | ||
with open(fragment_path2, "w") as f: | ||
f.write("Extends levitation") | ||
|
||
call(["git", "init"]) | ||
call(["git", "config", "user.name", "user"]) | ||
call(["git", "config", "user.email", "[email protected]"]) | ||
call(["git", "add", "."]) | ||
call(["git", "commit", "-m", "Initial Commit"]) | ||
|
||
result = runner.invoke(_main, ["--date", "01-01-2001", "--yes", "--keep"]) | ||
self.assertEqual(1, result.exit_code) | ||
|
||
result = runner.invoke(_main, ["--date", "01-01-2001", "--keep", "--yes"]) | ||
self.assertEqual(1, result.exit_code) | ||
|
||
def test_confirmation_says_no(self): | ||
""" | ||
If the user says "no" to removing the newsfragements, we end up with | ||
|
@@ -429,7 +489,7 @@ def test_confirmation_says_no(self): | |
call(["git", "add", "."]) | ||
call(["git", "commit", "-m", "Initial Commit"]) | ||
|
||
with patch("towncrier._git.click.confirm") as m: | ||
with patch("towncrier._remover.click.confirm") as m: | ||
m.return_value = False | ||
result = runner.invoke(_main, []) | ||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Close, but not quite what I had in mind.
We haven't really achieved anything by this refactoring, because this function still deletes the files which makes it an action with pretty heavy side-effects.
My suggestion was that this function returns the
fragment_filenames
as a list and the caller calls_git.remove_files()
on it unconditionally.Secondarily, you've moved this function into a separate module, so you can easily monkeypatch it without affecting the CLI entry point – correct?
That's pretty heavy "test damage".
How about going a bit functional here:
In the test you would simply pass
confirm_fn=lambda: True
orconfirm_fn=lambda: False
– look ma no patching!I would usually not suggest something like that on a public repo, but if the alternative is adding a new module for just one function that is called in one place, it seems worth it – would you agree?
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.
Hmmm.... you're right. I got lost a bit between reading your suggestion and sitting down to implement it.
When I look at it, the intermediate function only makes a decision on whether to remove the files or not. And would return the fragments_list or not.
I wonder if it wouldn't be even better if we had something like this:
Then both functions here could be tested separately. And... still, the should_remove_news_fragments could receive confirm_fn partial...
Sorry, I tend to split functions sooner, especially if the original module is already several hundred lines big. I'll move it back since... you're right, it's not called anywhere else, and that's a perfect rule to keep 🤔 .
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.
@hynek I don't think the functional approach will help in regards to the changed test.
The test that got the mock updated is an integration one (tests from all the way up by calling click command) and... I moved the click calls from
_git.remove
so unless I'll keep the intermediate within_git
module, you'd have to change the mock anyway 🤔And to actually make use of the functional approach the test itself would have to be a unit one.
I'm not sure how to approach it.
And the last question, shall I move the intermediate function into the build module?
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.
yeah the CLI entry points need to be refactored HARD, but let's not blow this up even further 😇
since the list is never changed, agreed on
should_remove_news_fragments
ok this click.confirm business is annoying. I was gonna suggest an abstraction, but I've noticed that it's literally only used in that one place so I'm gonna say to KISS and patch away on build.py without the confirm callable, unless you'd like to add more unittests.
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.
whew 😌
I can't think of any that the integration tests wouldn't already cover... so not at this time.
I'll try to remember next time I make a contribution though :)