Skip the checks if the news file was changed

[hexagonrecursion]

Closes #152
Closes #246
Closes #154

I wanted to use towncrier check as part of pip CI, but this should be fixed first.
This PR makes towncrier check succeed for a branch that changes the news file even if it does not add news fragments.

[codecov]

Codecov Report

Merging #337 (90005e1) into master (e0d72c4) will increase coverage by 0.03%.
The diff coverage is 98.41%.

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
+ Coverage   96.81%   96.85%   +0.03%     
==========================================
  Files          20       20              
  Lines        1194     1241      +47     
  Branches      118      120       +2     
==========================================
+ Hits         1156     1202      +46     
  Misses         20       20              
- Partials       18       19       +1     

Impacted Files	Coverage Δ
src/towncrier/test/test_check.py	99.21% <98.27%> (-0.79%)	⬇️
src/towncrier/check.py	94.44% <100.00%> (+0.44%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0d72c4...90005e1. Read the comment docs.

[hexagonrecursion]

Why are you testing on python2.7 if the minimum version you officially support is python3.5?
https://github.com/twisted/towncrier#quick-start

towncrier, as a command line tool, works on Python 3.5+ only.

[altendky]

Why are you testing on python2.7 if the minimum version you officially support is python3.5?
twisted/towncrier#quick-start

towncrier, as a command line tool, works on Python 3.5+ only.

Looks like the readme got updated at some point without updating the rest. It still works on 2.7 and I was planning on keeping that around until it becomes problematic. Correction submitted as #338. Not sure if there should be a 21.3.0post1 to address that on the PyPI page.

[altendky]

I think this PR relates to #309. I haven't been thinking about that for awhile. :[

[altendky]

FWIW, I do like pathlib. I'm not going to drop 2.7 just to get it, but there is https://pypi.org/project/pathlib2/ that I might be game for. Probably simplest though to just use os.path for now though. Though until we figure out #309, I suspect this PR might get hung up. Though I hate to have that be the case.

[adiroiban]

Thanks Andrey for the nice PR.

I like the idea of using the NEWS file update as a hint that you are on a release branch.
I don't see any issues with that.

---

I would go even further and fail in there are news fragment and NEWS file is updated at the same time.... but this can be a separate PR.

---

I think that this also fixes #154

Yes. It's related to #309 .

[adiroiban]

Thanks for the PR and the documentation.

I left an initial set of comments.

I would be awesome to all see check fails if there are both NEWS changes and newsgragment files present... but not required for this PR.

I am waiting for @altendky to see how he thinks about this PR

[adiroiban]

This looks like duplicate code between tests.
Why not reuse the existing create_project

Suggested change

	# Arrange
	write("towncrier.toml", "[tool.towncrier]")
	write("newsfragments/.gitignore", "!.gitignore")
	initial_commit()
	create_project('towncrier.toml')

[hexagonrecursion]

I dislike create_project().

• It encapsulates a poorly scoped amount of code. The semantics are unclear. It does too many unrelated things. Changing it like you did in Make check subcommand detect if only configured news file has changed #296 affects every test that uses it which is risky.
• It is inflexible: you either use it or you don't. I prefer my approach: a set of (mostly) orthogonal building blocks that every test can use to arrange things precisely the way that particular test needs to.
• It is a little to implicit for my taste. I actually like that every file the test creates is explicitly named in the test itself.

[hexagonrecursion]

Changing it like you did in #296 affects every test that uses it which is risky.

Just to be clear: I'm not criticizing you, I'm criticizing a specific decision you made because I think I know how to solve that specific problem better.

[adiroiban]

@hexagonrecursion don't worry :) If you think that some code is not ok. please say it loud.
I will not take it as an offense... I know that I am capable of writing bad code :)

Your comments are valid.

Regarding the risky change. As long as the tests are not failing, I think that any change should be done if we thing that it improves the readability and usability.
We should be confident that the test suite is solid and don't code with fear :)

If you think that create_project is bad pattern, it's best to add a docstring comment to it and let other developers know what is the current best practice.

But I think that all test helpers should have short docstrings describing when and why they should be used :)

[hexagonrecursion]

I know that I am capable of writing bad code

Same here. I also prefer to err on the side of not offending anybody.

If you think that create_project is bad pattern, it's best to add a docstring comment to it and let other developers know what is the current best practice.

If you are onboard with my argument I'll submit a followup PR to refactor the rest of the file and delete create_project()

But I think that all test helpers should have short docstrings describing when and why they should be used :)

👍

[adiroiban]

Do we need this ignore here?
I guess that this is a replacement for write("newsfragments/123.feature", "Foo the bar")
For me, writing an explicit news fragment is easier to read and understand what is going on here.

[adiroiban]

A brief summary of an IRC chat with Kyle.

My preference is to have a separate towncrier check-releasethat is explicitly triggered for a release branch.
It will check that NEWS is updated and no fragments are left in the branch.
In this way, the normal towncrier check will fail if you are not on a release branch and you accidentally modify the news file without create a news fragment.

I understand that having a modified NEWS file means 99.99% that you are on a release branch and this is why I think that this behavior is also.

At the same time, by just using check you get a simple solution that just works and a modified NEWS file is a good heuristic for a release branch (99.99%).

Later we can add towncrier check --strict if required.

I am waiting for @altendky to weight his opinion on this :)

[hexagonrecursion]

codecov complains that a line in the write() helper is not covered. Should I write a test for the helper?

[adiroiban]

I would say no... and just add a pragma coverage comment, and enable them in .coveragerc

[altendky]

@hexagonrecursion, my apologies for not getting back to you on this sooner. I am with Adi on this one in terms of preferring an explicit selection of the mode of the check. That said, I think we can start with the implicit command and add the explicit later whenever those of us that want it find the time to do so. We already have towncrier check. In a comprehensive solution we could have subcommands towncrier check auto, release, and non-release (or whatever names, I don't mean to suggest these are the best names).

I am not convinced that 'the newsfile has been edited' is a great heuristic. There have been a number of times where I have edited a newsfile to fix it. If we can make this a provisional feature then I'll be less picky, but I'm not sure that is plausible as the default behavior for an existing command. That leads to either it being configurable or a dedicated command. It doesn't seem like it should be too problematic to introduce this as a new feature while leaving the existing behavior intact.

My gut, at the moment, says to add two subcommands (existing behavior as default, other subcommand being the new behavior). We can add the explicit release check command separately, or here, no real matter. Then, when another heuristic is developed, we can add a configuration setting for which one to use with news-file-modified being the default. I think that this will be a small amount of added work, will retain backwards compatibility, and will enable future expansion. Does anyone agree?

To avoid dumping more work on you, @hexagonrecursion, I'll volunteer to rework the commands either before or after merging this, if you prefer.

I hope this comes across as appreciation for your work and an effort to figure out how to make sure we put it to the best use for all. When I released 21.3.0 it was actually missing one of... I think two (?) things that I had set out to include initially. This was one of them. So, I'm looking forward to getting this figured out. Thank you. :]

[adiroiban]

doing a release for towncrier itself using tox -e check-newsfragment is a pain.

so, even if this doesn't solve all the problems, I think that it should be updated to pass all tests and then merged.

[adiroiban]

@hexagonrecursion sorry for the whole delay.
let me know if you still want to drive this PR.

Otherwise I can try to take over and merge it.

Thanks!

[hexagonrecursion]

Otherwise I can try to take over and merge it.

Go ahead.

[adiroiban]

Hi @hexagonrecursion

The test on your branch were failing so I had to do some debugging and see why.

During the debugging session I did a bit of refactoring to reduce code duplication and help me understand what is going on there.

Please check the latest 2 commits.

If you are happy with them, we can merge this.

Thank you all for the work on this!

[hexagonrecursion]

Regarding the risky change. As long as the tests are not failing, I think that any change should be done if we thing that it improves the readability and usability.
We should be confident that the test suite is solid and don't code with fear :)

The tests are our safety net when we refactor code, but we have no such luxury when we refactor the tests themselves. Changing create_project() might lead to tests no longer testing what they intended to test and possibly passing when they should fail.

We should refactor tests, but the refactoring should usually be limited to one of two types:

1. True refactoring - the implementation is changed, but the observable behavior stays the same
2. Refactoring of a specific test that makes sure to preserve intention of the test

A "refactoring" that changes the setup used in many tests requires an audit of every affected test to make sure it still tests the behavior it intended to test

[hexagonrecursion]

You deleted the "Arrange" comment, but left "Act" and "Assert". It would be more consistent to either have all 3 or none of them.

[adiroiban]

I will add Arrange back. Thanks!

[hexagonrecursion]

You have refactored 4 lines to 2 lines. IMHO you have reduced the clarity of the test in the process:

1. To understand it I now have to reference create_project()
2. The reader also has to figure out which parts of create_project() are relevant to a given test and which parts are not and why

[hexagonrecursion]

Also: create_project() suffers from a lack of orthogonality - it glues together 3 loosely related pieces of setup into a very specific shape:

• The creation of 3 files
• A commit
• A switch to a different branch

[adiroiban]

The intention is to try to reuse the create_project so that the tests will use the same same set of conventions when managing the news files.

So, to increase consistency.

I am not happy with create_project, but the idea is that when you work on these tests, you understand what create_project does once, and then you should have a good idea of what to expect in each test.

[hexagonrecursion]

"Sync release in main branch."

should probable say "Merge new-feature-branch" or "Merge: Foo the bar" because we are merging a feature branch rather than a release branch

[adiroiban]

Thanks. I will update.

[hexagonrecursion]

Fix the test.

There is nothing in the commit message explaining (I) what was broken (II) how did your change fix it. Also: the refactoring should have been a separate commit before or after the fix to make it more obvious which changes are a part of the fix

[adiroiban]

The tests are our safety net when we refactor code, but we have no such luxury when we refactor the tests themselves.

I think that we should take into consideration that this is a hobby project,
for which people are contributing in their free time.

Also, we are talking here about towncrier which is a tool to combine a set of text files into a single one.
It's also a tool that is used only by the developers and not triggered by end users.
Is also not a tool distributed to end users.
I don't think that is OK to apply the same set of development requirements for towncrier as a software that will drive a robot on Mars.

I think that we are OK to go with a more relaxed approach.

I am not saying that we should ignore all the best practices :)

There is nothing in the commit message explaining (I) what was broken (II) how did your change fix it. Also: the refactoring should have been a separate commit before or after the fix to make it more obvious which changes are a part of the fix

True. I will try to separate the changes for the next PR and find better description.
For work I merge with squash, so I don't put much effort into commit messages.

But I think that the diff is not that big... is not idea, but not that bad :)

[adiroiban]

Hi @hexagonrecursion Many thanks for the review and sorry for the mess.

If you think that the last refactoring is that bad, I am happy to have it reverted and have the tests fixed in another way.

Cheers

[hexagonrecursion]

It could better, but it is OK. I am not against merging it.

[adiroiban]

Many thanks for the excellent review and your support for improving the quality of the project.

I think that we can at any time have a new PR to improve the current testing... but time is short and I think that for towncrier "done is better than perfect".

I did the last release of towncrier, and without this feature, the release process was a bit of a PITA :)

Happy to see this merge and hope it will improve the next release... and I plan to release soon :)