Make check subcommand detect if only configured news file has changed #296
Conversation
This should enable the check subcommand to be used as a CI lint step and not fail when a pull request only modifies the configured news file (i.e. when the news file is being assembled for the next release).
Codecov Report
@@ Coverage Diff @@
## master #296 +/- ##
==========================================
+ Coverage 96.81% 96.86% +0.04%
==========================================
Files 20 20
Lines 1194 1212 +18
Branches 118 119 +1
==========================================
+ Hits 1156 1174 +18
Misses 20 20
Partials 18 18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank for putting this together.
I am happy to see this functionality in towncrier.
I am not sure if this will work.
I don't know what is the "configured news" file.
I guess that it will work for a very limited use case.
For a release PR, I am also updating a file containing the version (setup.py / setup.cfg or /_version.py) and sometimes
And basically, for a release branch I am doing the reverse check.
The news fragments check will fail if they is any newsfragment at all.
My suggestion is to rename to "Make --check work for a release branch." or "Detect when running on a release branch."
Then for a release branch it will not fail if there is no news fragment for the branch, but will fail if there are any newsfragments at all.
I see 2 tasks:
- detect (heuristic) if towncrier runs for a release branch
- run different checks for a release branch
One idea is to add a separate --check-release argument that will run point 2.
In this way, for point 1 users will add their own external logic to detect a release branch.
I am detecting the release branch based on the branch name (*-release-) , it works for my buildbot CI. Not sure if it works with GitHub.
I am using this helper https://github.com/chevah/brink/blob/master/brink/pavement_commons.py#L129
My branches all start with the ticket id, in this way is easier to detect so I can also check that the file has the right news fragment...and not just any news fragment :)
| @@ -55,6 +64,10 @@ def __main(comparewith, directory, config): | |||
| click.echo("On trunk, or no diffs, so no newsfragment required.") | |||
| sys.exit(0) | |||
|
|
|||
| if files_changed == config["filename"]: | |||
| click.echo("Only the configured news file has changed.") | |||
There was a problem hiding this comment.
I would say add uppercase when skipped, to help detect accidental configuration errors.
| click.echo("Only the configured news file has changed.") | |
| click.echo("Checks SKIPPED. Only news file changes detected that looks like a release branch.") |
| @@ -13,7 +14,11 @@ | |||
|
|
|||
| def create_project(pyproject_path): | |||
| with open(pyproject_path, "w") as f: | |||
| f.write("[tool.towncrier]\n" 'package = "foo"\n') | |||
| f.write(dedent("""\ | |||
There was a problem hiding this comment.
I think that we should disable the linter warning for this type of indentation... and maybe just use black :)
I feel that dedent here has no technical merit, and is just added to please a tool :)
|
|
||
| def test_newsfile_modified(self): | ||
| """ | ||
| A modified newsfile satisfies the check. |
There was a problem hiding this comment.
| A modified newsfile satisfies the check. | |
| When only the newsfile is modified on a branch --check will not complain about missing newsfile fragmants. |
There was a problem hiding this comment.
I don't understand the general concerns about functionality and what the configured newsfile is. Isn't the configured newsfile the one that would be written to if using build instead of check? Perhaps this is just misuse of terminology where newsfile is being improperly used where changelog should be instead. In my head news fragments were used to build a news file (often called a changelog). But, I see in the readme These fragments are also commonly called "topfiles" or "newsfiles" in Twisted parlance. The readme doesn't seem to use any distinct name for the output file beyond the filename configuration option name. This is probably a thing to address.
Maybe that clarification helps explain the concept? The intent was that check would check for either a modification to the output file (changelog?) and no modified newsfragments, or one or more added news fragment files. I see that this will fail in the case that you otherwise edit the output file and also have a news fragment. Also, it doesn't check for an actual lack of news fragments when doing 'the release check'.
I appreciate the interest in explicit external control. I think something like a regex in the configuration file to choose based on branch name or such might be a reasonable option as well. I'll take a step back and see what I can come up with.
As always, thanks for the consideration and suggestions.
|
Note that I was not rejecting the PR :) Just adding a comment with feedback based on my usage of towncrier and my release procedure. I call the resulting file "Release Notes" .I find that end users / customers understand it best (vs NEWS File (GNU) or Changelog File). I guess that we can also add the case when only the "newsfile" is modified in a branch and disable the checks in that case. Maybe start small by adding a special |
Sometimes I feel that the comments are sensibly addressed in a follow up and that the existing PR is a good stepping stone. Per my thoughts after reading your comments, this PR doesn't seem to get either the branch detection bit right nor implement the proper checks for each case. I think these points should be addressed instead of merging this (roughly) as is. I think the separation of implementing the checks and the branch detection make sense. This PR may just get closed and replaced by two to cover those separate pieces. We'll see when I make it back to working on this one. |
|
I think that this is now fixed with #337 |
Closes #152
Closes #246
Draft for: