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

[fizyk]

…ts - closes #129

Description

Resolves issue #129 It's not a --no option, but given the generality of the --no option, --keep is a bit more descriptive than proposed --no.

Checklist

• Make sure changes are covered by existing or new tests.
• For at least one Python version, make sure local test run is green.
• Create a file in src/towncrier/newsfragments/. Describe your
  change and include important information. Your change will be included in the public release notes.
• Make sure all GitHub Actions checks are green (they are automatically checking all of the above).
• Ensure docs/tutorial.rst is still up-to-date.
• If you add new CLI arguments (or change the meaning of existing ones), make sure docs/cli.rst reflects those changes.
• If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

[codecov]

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (c5dadf0) compared to base (9554985).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             trunk      #453   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         1607      1658   +51     
  Branches       286       297   +11     
=========================================
+ Hits          1607      1658   +51     

Impacted Files	Coverage Δ
src/towncrier/_git.py	100.00% <100.00%> (ø)
src/towncrier/build.py	100.00% <100.00%> (ø)
src/towncrier/test/test_build.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[adiroiban]

Hi Grzegorz. Many thanks for the nice PR.

I don't really understand why this is needed.

There is the "draft" command which is expected to generate the release notes, without removing the fragments.

It is true that right now, the draft command generates the result in the standard output.

But maybe a better way to fix this is to add a file option to the existing "draft" command.

This is just a suggestion.

[fizyk]

@adiroiban I do not have strong opinion on where. It seemed natural, although the --draft could also be the solution.

The underlying issue suggests it would be useful to generate a current changelog for development documentation, my issue is that having numerous fragment types I want to have a general changelog with all the changes and additionally specific categories being generated into its own, separate changelog file as well.

[adiroiban]

Thanks for the explanation. I think '--keep' makes sense.

The underlying issue suggests it would be useful to generate a current changelog for development documentation,

I think that this is "draft" use case.
But "draft" has clear documentation that it sends the result to stdout... so... it's complicated.

I will check the PR now and I hope we can merge it.

Thanks again

[adiroiban]

This looks very good. Many thanks for writing the docs and tests.

It helps a lot with the review.

I left a few minor comments.

The biggest comment is the lack of docstring for the new tests.
Please use it to describe the targeted end-user functionality.

The towncrier project don't really has a good knnowledge of what it should do and how it should do.
Towncrier is pretty flexible and various people are using it for their use cases... and without a docstring for a tests, it's hard to know what is a side effect and what is an intended feature.

---

I understand the use case, and I think that this is specific enough not to be included in the tutorial.

---

If you add new configuration options (or change the meaning of existing ones), make sure docs/configuration.rst reflects those changes.

@hynek I think that here we should say

-If you add new configuration options
+If you add new template options

This PR has only CLI options so I think that we are fine.

[adiroiban]

Thanks. Changes looks good.
Only very minor comments that should not be a blocker.

Somehow, I have messed up the coverage reporting back to GitHub PR.
The coverage is ok, but posting back the comment fails.

I will try to fix trunk and then we can merge this.

Many thanks again for your help!

[adiroiban]

I/we still need to fix the coverage reporting issue.

I did an initial investigation, and it looks like forks only get read permission

I can see that for the main repo, the permissions are write ... for example for the latest trunk merge

https://github.com/twisted/towncrier/actions/runs/3404794371/jobs/5662304592#step:1:17

But for this PR the permissions are only read

https://github.com/twisted/towncrier/actions/runs/3563302652/jobs/5985987960#step:1:17

@hynek @altendky Do you any a quick fix for standalone coverage reporting on forks ?

Thanks!

[hynek]

Maybe https://github.com/py-cov-action/python-coverage-comment-action? I think actions have different token handling

[adiroiban]

I think actions have different token handling

As far as I know, the actions are just a "step" , so they run in the context of the job.

We are using the default token...and the default token only has read permissions.
As documented here

There is this setting here
but I don't know what are the side effects

[hynek]

@adiroiban sadly I'm rather clueless at all these things. If you want to just quickly unblock contributions I can offer my method that doesn't comment, but at least works for now. People just have to look into the CI output for missing lines.

[adiroiban]

Thanks @hynek . Yes, the commit status might be the only "safe" option.

In theory, we could give write access to forks, since our workflow has explicit permissions... but if in the future a new workflow is created without explicit permissions, this can lead to a security void, as we might accidentally give full access to the repo to forks.

At the same time, I think that new contributors need to have their first PR manually approved, so the workflow code is not executed right away, unattended.

I will look to create a new PR that will always send the commit status and will post a comment only for non-forks.

[hynek]

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?

[fizyk]

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

[fizyk]

@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?

[hynek]

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.

[fizyk]

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

[hynek]

sorry for the delay, I've been sick all week.

i think given the circumstances this is good to go.

[adiroiban]

Many thanks @fizyk for the changes and @hynek for the review.
I was busy at work pushing a review before the holiday break.

I think we can also look at pushing a new release for towncrier... to get this feature and also get latest documentation

[hynek]

Agreed – having the news docs would be great!

[fizyk]

@hynek @adiroiban thank you both for the review, discussion and satisfactory work :)
And I'm glad I could also close one issue from someone's issue board :)