Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add --keep option to allow to generate newsfile, but keep newsfragmen… #453

Merged
merged 9 commits into from Dec 19, 2022
Merged

Add --keep option to allow to generate newsfile, but keep newsfragmen… #453

Show file tree
Hide file tree
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 Nov 24, 2022
0ee66f1
Apply suggestions from code review
fizyk Nov 25, 2022
3b44cec
Additional post-PR enhancements
fizyk Nov 25, 2022
6dc3e63
Update build --keep comment to be more relevant
fizyk Nov 28, 2022
bf049f7
More compact remove_files and use with_isolated_runner for new tests
fizyk Nov 28, 2022
0445078
Refactored _git.remove_files
fizyk Dec 1, 2022
1908de0
Update src/towncrier/newsfragments/129.feature
fizyk Dec 5, 2022
ab17b50
Make a decision and return it instead of calling _git.remove underneath
fizyk Dec 5, 2022
c5dadf0
Move should_remove_fragment_files into build module
fizyk Dec 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/cli.rst
View file
Open in desktop
Expand Up @@ -50,6 +50,10 @@ Build the combined news file from news fragments.
Do not ask for confirmations.
Useful for automated tasks.

.. option:: --keep

Don't delete news fragments after the build and don't ask for confirmation whether to delete or keep the fragments.


``towncrier create``
--------------------
Expand Down
17 changes: 2 additions & 15 deletions src/towncrier/_git.py
View file
Open in desktop
Expand Up @@ -7,22 +7,9 @@

from subprocess import STDOUT, call, check_output

import click


def remove_files(fragment_filenames: list[str], answer_yes: bool) -> None:
if not fragment_filenames:
return

if answer_yes:
click.echo("Removing the following files:")
else:
click.echo("I want to remove the following files:")

for filename in fragment_filenames:
click.echo(filename)

if answer_yes or click.confirm("Is it okay if I remove those files?", default=True):
def remove_files(fragment_filenames: list[str]) -> None:
if fragment_filenames:
call(["git", "rm", "--quiet"] + fragment_filenames)


Expand Down
30 changes: 30 additions & 0 deletions src/towncrier/_remover.py
View file
Open in desktop
@@ -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:
Copy link
Member

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:

_git.remove_files(
    find_news_fragment_files_for_removal(  # <-- better name for remove_news_fragment_files after changes
        fragment_filenames, answer_yes, answer_keep,
        confirm_fn=partial(click.confirm, "Is it okay if I remove those files?", default=True)
    )
)

In the test you would simply pass confirm_fn=lambda: True or confirm_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?

Copy link
Contributor Author

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:

if should_remove_news_fragment(fragment_filenames, answer_yes, answer_keep):
    _git.remove_files(fragment_files)

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 🤔 .

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I tend to split functions sooner, especially if the original module is already several hundred lines big.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but let's not blow this up even further

whew 😌

unless you'd like to add more unittests

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 :)

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)
34 changes: 31 additions & 3 deletions src/towncrier/build.py
View file
Open in desktop
Expand Up @@ -15,8 +15,12 @@

import click

from click import Context, Option

from towncrier._remover import remove_news_fragment_files

from ._builder import find_fragments, render_fragments, split_fragments
from ._git import remove_files, stage_newsfile
from ._git import stage_newsfile
from ._project import get_project_name, get_version
from ._settings import ConfigError, config_option_help, load_config_from_options
from ._writer import append_to_newsfile
Expand All @@ -26,6 +30,18 @@ def _get_date() -> str:
return date.today().isoformat()


def _validate_answer(ctx: Context, param: Option, value: bool) -> bool:
value_check = (
ctx.params.get("answer_yes")
if param.name == "answer_keep"
else ctx.params.get("answer_keep")
)
if value_check and value:
click.echo("You can not choose both --yes and --keep at the same time")
ctx.abort()
return value


@click.command(name="build")
@click.option(
"--draft",
Expand Down Expand Up @@ -67,9 +83,18 @@ def _get_date() -> str:
@click.option(
"--yes",
"answer_yes",
default=False,
default=None,
flag_value=True,
help="Do not ask for confirmation to remove news fragments.",
callback=_validate_answer,
)
@click.option(
"--keep",
"answer_keep",
default=None,
flag_value=True,
help="Do not ask for confirmations. But keep news fragments.",
callback=_validate_answer,
)
def _main(
draft: bool,
Expand All @@ -79,6 +104,7 @@ def _main(
project_version: str | None,
project_date: str | None,
answer_yes: bool,
answer_keep: bool,
) -> None:
"""
Build a combined news file from news fragment.
Expand All @@ -92,6 +118,7 @@ def _main(
project_version,
project_date,
answer_yes,
answer_keep,
)
except ConfigError as e:
print(e, file=sys.stderr)
Expand All @@ -106,6 +133,7 @@ def __main(
project_version: str | None,
project_date: str | None,
answer_yes: bool,
answer_keep: bool,
) -> None:
"""
The main entry point.
Expand Down Expand Up @@ -237,7 +265,7 @@ def __main(
stage_newsfile(base_directory, news_file)

click.echo("Removing news fragments...", err=to_err)
remove_files(fragment_filenames, answer_yes)
remove_news_fragment_files(fragment_filenames, answer_yes, answer_keep)

click.echo("Done!", err=to_err)

Expand Down
1 change: 1 addition & 0 deletions src/towncrier/newsfragments/129.feature
View file
Open in desktop
@@ -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
62 changes: 61 additions & 1 deletion src/towncrier/test/test_build.py
View file
Open in desktop
Expand Up @@ -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", "user@example.com"])
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", "user@example.com"])
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
Expand All @@ -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, [])

Expand Down