-
-
Notifications
You must be signed in to change notification settings - Fork 625
Setup Towncrier for changelog management, remove release-drafter #2203
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
base: main
Are you sure you want to change the base?
Conversation
Modeled on a combination of `cheroot/towncrier.toml` and `attrs/changelog.d/towncrier_template.md.jinja`, these files are an initial configuration with minimal complexity. For example, the `cheroot` code block which looks up issue numbers is omitted for simplicity. An initial basic `tox r -e changelog` environment is added here to let us verify the config. The contribution doc is modeled on the `attrs` one.
- add the sphinx extension - setup detection of release builds - inject towncrier draft content into the changelog only if it is not a release
Modeled on the hook from cherrypy/cheroot[^1]. [^1]: https://github.com/cherrypy/cheroot/blob/fdaa24d8470d68341a84d977f32436e1344e8230/.pre-commit-config.yaml#L9-L49
This ignores all files except for changelog files (and the static contents of the dir), making it more difficult to add non-changelog content.
This follows the example of cherrypy/cheroot, but with a few alterations: - no need to specially handle `int` in jinja, since the bug referenced in current cheroot config has been fixed - match the current PR style which puts issue links in parens on the same line as the text - only put one line between items; closer to current style
These are compiled from the non-automated pull requests which have merged since v7.4.1 (the last release).
Use a subprocess invocation to control stderr with the fewest additional dependencies and minimal complexity.
@sirosen I was thinking about having the change log fragments dir nested under the docs dir to reduce the number of the high-level things. Any objections? |
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.
This is not very in-depth. I'll do another round after your next update. Could you make sure the contributors are credited in the notes, too?
|
||
##### Maintainer checklist | ||
|
||
- [ ] Verified one of these labels is present: `backwards incompatible`, `feature`, `enhancement`, `deprecation`, `bug`, `dependency`, `docs` or `skip-changelog` as they determine changelog listing. | ||
- [ ] If no changelog is needed, apply the `skip-changelog` label. |
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.
We might need to record some guidelines for this (maybe, in a follow-up).
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.
Yes, I think we can refine this more in the contributing docs? For now, I wanted to make some kind of update here and keep it pretty simple.
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.
Yes, I was imagining it'd be just linked here.
@@ -3,9 +3,9 @@ | |||
##### Contributor checklist | |||
|
|||
- [ ] Included tests for the changes. | |||
- [ ] PR title is short, clear, and ready to be included in the user-facing changelog. | |||
- [ ] A newsfile is created in `changelog.d/` (see `changelog.d/README.md` for instructions) or the PR text says "no changelog needed". |
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.
Do we want to call this newsfiles or change notes / fragments for consistency?
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.
I was trying to be consistent in calling them "newsfiles" since that seems to be the terminology in Towncrier.
"Change notes" may be more contributor-friendly though, for those who aren't familiar with the tool.
I have no strong opinion on this. Since you've raised it, I think I'll switch it to "change notes" and apply that elsewhere as well.
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.
Yep, that's the reason I've been using that terminology.
@@ -0,0 +1,57 @@ | |||
*{{ versiondata["date"] }}* |
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.
Don't we have to render the actual version title? Looks like it's missing. Never mind the sphinxcontrib-towncrier
preview. It may be quirky.
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.
Oh, yep, this looks like a mistake. I noticed the version was not visible in the cheroot
template and didn't understand quite what might be happening there. But for this case we should have it.
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.
I think that old versions of Towncrier would prepend the version title outside Jinja and than it changed. The recent Sphinx plugin version doesn't support those old versions of Towncrier anymore.
Also, the plugin adds a title IIRC, needs checking but I wouldn't worry about the preview. Making sure that the regular release run puts it in place is what's most important.
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.
Could you use symlinks instead of having copies of fragments?
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.
Happy to oblige! I'll do a cleanup pass for that.
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.
Thanks!
-r{toxinidir}/docs/requirements.txt | ||
commands = | ||
towncrier --version | ||
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"], stderr=subprocess.PIPE)' |
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.
Is this meant to eat stderr? Why wrap it in this state? Also, I usually have posargs IIRC, that allow me to pass args for experiments.
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.
You had done something similar:
https://github.com/cherrypy/cheroot/blob/fdaa24d8470d68341a84d977f32436e1344e8230/tox.ini#L336-L346
You used sh
, and I slightly prefer this subprocess usage but don't feel strongly.
At first, I tried without this, thinking it wasn't strictly necessary. But running several times, I found that stderr sometimes was interleaved with stdout. I definitely should add a comment on it though.
If we're wrapping it, I feel like {posargs}
gets a little messy. I guess it could be done with...
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"], stderr=subprocess.PIPE)' | |
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"] + sys.argv[1:], stderr=subprocess.PIPE)' {posargs} |
that? I think that works. I'll try it.
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.
Right. I forgot about it. Your way is cleaner. Although, I'd probably pass subprocess.DEVNULL
there instead. subprocess.PIPE
confused me because what it exposes ended up being unused. Also, you probably need check=True
to replicate the same behavior so that Python wouldn't eat the underlying return code. I'd go for subprocess.check_call()
, though, as it encapsulates it. And my Cheroot variant was also adding -bb -I
that would be nice to keep them too.
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.
Something like this
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"], stderr=subprocess.PIPE)' | |
{envpython} -bb -E -s -I -c 'import sys, subprocess; subprocess.check_call([sys.executable, "-bb", "-E", "-s", "-m", "towncrier", "build", "--version", "main", "--draft"] + sys.argv[1:], stderr=subprocess.DEVNULL)' {posargs} |
- For example, if your PR fixes bug #404, the newsfile should be named | ||
`changelog.d/404.bugfix.md`. | ||
|
||
- If multiple issues are addressed, make an identical copy of the newsfile with |
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.
I'd prefer a symlink, though.
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.
Yep, consistent with the note on it above, I'll not just fix my own copies, but also this bit of contributor doc.
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.
Perfect!
|
||
- Prefer the simple past or constructions with "now". | ||
|
||
You can preview the changelog by running `tox run -e changelog-draft`. |
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.
I would probably tell the users to build the Sphinx docs and see it rendered in the surrounding context.
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.
Good idea! I prefer this as well, since it will also help them spot malformed content.
I still think this is handy for maintainers, but we can direct users to tox run -e build-docs
.
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.
Yes. Though, that tox env was originally intended for certain GHA integrations IIRC. I haven't asked to implement that in the issue since that would extend the scope and complexity. Perhaps in the future..
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.
Did you decide not to follow my boilerplate? Any thoughts? Concrete fragment examples to show? These could be a few of the fragments you're adding in this PR copied into this doc.
Also, let's mention the possibility of bylines.
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.
First, regarding bylines: yes, I'll add that for sure. More thoughts on that topic to follow (I want to rewrite the past entries too).
I was trying to balance concerns between trying to keep things similar to what you have in other projects, but also produce a document which gets new contributors to the instructions quickly. I'm a little concerned about the example from cheroot
-- I think that for a new contributor that doc is a bit intimidating and gives too much detail about the rationale for Towncrier and changelogs up front.
My preference is for this doc to read primarily as a "how to", and for the motivation behind it to be separated out. (Similar to diataxis, but simply as sections within a doc.)
I think we can reintroduce all of the elments I removed in a sub-section, keeping the start of the doc short and recipe-like.
Would you be okay with the content flowing as follows?
## Adding Change Notes with PRs
<mostly the currently proposed "how to" content + mention of adding a byline>
### Categories
The change categories are defined as follows:
- `bugfix:` A bug fix for something we deemed an improper undesired behavior that got corrected in the release to match pre-agreed expectations.
- ... <list follows>
Sometimes it's not clear which category to use for a change. Do your best and a maintainer can discuss this with you during review.
### Example Change Notes
<copy from cheroot example>
### Rationale
When somebody makes a change, they must record the bits that would affect end-users only including information that would be useful to them. ... <content from cheroot example follows>
We write change notes in the past tense because this suits the users who will be reading these notes.
Combined with others, the notes will be a part of the "news digest" telling the readers what **changed** in a specific version of the library since the previous version.
<anything else from the cheroot doc which I've missed probably goes here>
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.
I like this idea. I was mostly writing those explanations in isolation with no feedback loop, having been tired of re-explaining how people should author their notes. And so I've been dragging this document around across projects.
I think it's imperfect, and I'd like to improve it. I've been thinking of how to integrate some ideas from https://redhat-documentation.github.io/supplementary-style-guide/#release-notes and lurking into the release notes channel in the Write The Docs Slack, but haven't gotten to it yet.
So if you manage to make it more friendly, I'd love to copy it back into other projects. Making sphinxcontrib-towncrier
was one of the sides of me attempting to improve authoring change logs.
### Example Change Notes <copy from cheroot example>
I think you should copy some of the actual change notes you're adding in this PR. I've been doing that, and it feels more immersive when yarl fragment examples talk about yarl and aiohttp examples talk about aiohttp.
I'd be nice to improve my bot eventually to be able to help with change note authoring, similar to CPython's blurb-it
. But that's a separate endeavor.
@@ -0,0 +1 @@ | |||
`pip-tools` now uses `sphinx-issues` to link to issues, PRs, commits, and user accounts. |
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.
Where possible, I'd add links to external projects + bylines.
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.
Sure! Since there's no docs site to link, I'll point to the sphinx-issues repo.
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, either repo or PyPI is fine with me. I just like having more context available where possible.
@@ -0,0 +1,3 @@ | |||
Requirements file paths in `pip-compile` output are now normalized to | |||
posix-style, even when `pip-compile` is run on Windows. |
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.
posix-style, even when `pip-compile` is run on Windows. | |
POSIX-style, even when `pip-compile` is run on Windows. |
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.
Could you make sure the contributors are credited in the notes, too?
On review, I don't like my decision not to include the Thanks <user>
lines, as with the past changelog entries.
I think I was struggling with the fact that there's no trivial way to do this in the format that I converted the past changelog entries to use. (Where the "Thanks" comes after the issue reference).
I'll include it here and I think I'll also do a separate change to make the changelog match the "new" format, of
- <text>. Thanks <user>!
(#NNN, #NNN)
I want things to read uniformly if possible, from old to new. :)
I was thinking about having the change log fragments dir nested under the docs dir to reduce the number of the high-level things. Any objections?
I somewhat prefer to have it in the root of the repo, but that's weakly held. If your preference here is a strong one, I can move it.
My rationale is that this is a touch-point for any contributor, so it's simpler (especially for a new contributor) to only traverse one dir. And I think changelogs are important enough to get "two slots" (the file and the dir) in the repo root.
I can't put the time in right now to make the suggested changes, but I'll get it done soon. 👍
|
||
##### Maintainer checklist | ||
|
||
- [ ] Verified one of these labels is present: `backwards incompatible`, `feature`, `enhancement`, `deprecation`, `bug`, `dependency`, `docs` or `skip-changelog` as they determine changelog listing. | ||
- [ ] If no changelog is needed, apply the `skip-changelog` label. |
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.
Yes, I think we can refine this more in the contributing docs? For now, I wanted to make some kind of update here and keep it pretty simple.
@@ -3,9 +3,9 @@ | |||
##### Contributor checklist | |||
|
|||
- [ ] Included tests for the changes. | |||
- [ ] PR title is short, clear, and ready to be included in the user-facing changelog. | |||
- [ ] A newsfile is created in `changelog.d/` (see `changelog.d/README.md` for instructions) or the PR text says "no changelog needed". |
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.
I was trying to be consistent in calling them "newsfiles" since that seems to be the terminology in Towncrier.
"Change notes" may be more contributor-friendly though, for those who aren't familiar with the tool.
I have no strong opinion on this. Since you've raised it, I think I'll switch it to "change notes" and apply that elsewhere as well.
@@ -0,0 +1,57 @@ | |||
*{{ versiondata["date"] }}* |
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.
Oh, yep, this looks like a mistake. I noticed the version was not visible in the cheroot
template and didn't understand quite what might be happening there. But for this case we should have it.
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.
Happy to oblige! I'll do a cleanup pass for that.
@@ -0,0 +1 @@ | |||
`pip-tools` now uses `sphinx-issues` to link to issues, PRs, commits, and user accounts. |
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.
Sure! Since there's no docs site to link, I'll point to the sphinx-issues repo.
- For example, if your PR fixes bug #404, the newsfile should be named | ||
`changelog.d/404.bugfix.md`. | ||
|
||
- If multiple issues are addressed, make an identical copy of the newsfile with |
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.
Yep, consistent with the note on it above, I'll not just fix my own copies, but also this bit of contributor doc.
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.
First, regarding bylines: yes, I'll add that for sure. More thoughts on that topic to follow (I want to rewrite the past entries too).
I was trying to balance concerns between trying to keep things similar to what you have in other projects, but also produce a document which gets new contributors to the instructions quickly. I'm a little concerned about the example from cheroot
-- I think that for a new contributor that doc is a bit intimidating and gives too much detail about the rationale for Towncrier and changelogs up front.
My preference is for this doc to read primarily as a "how to", and for the motivation behind it to be separated out. (Similar to diataxis, but simply as sections within a doc.)
I think we can reintroduce all of the elments I removed in a sub-section, keeping the start of the doc short and recipe-like.
Would you be okay with the content flowing as follows?
## Adding Change Notes with PRs
<mostly the currently proposed "how to" content + mention of adding a byline>
### Categories
The change categories are defined as follows:
- `bugfix:` A bug fix for something we deemed an improper undesired behavior that got corrected in the release to match pre-agreed expectations.
- ... <list follows>
Sometimes it's not clear which category to use for a change. Do your best and a maintainer can discuss this with you during review.
### Example Change Notes
<copy from cheroot example>
### Rationale
When somebody makes a change, they must record the bits that would affect end-users only including information that would be useful to them. ... <content from cheroot example follows>
We write change notes in the past tense because this suits the users who will be reading these notes.
Combined with others, the notes will be a part of the "news digest" telling the readers what **changed** in a specific version of the library since the previous version.
<anything else from the cheroot doc which I've missed probably goes here>
|
||
- Prefer the simple past or constructions with "now". | ||
|
||
You can preview the changelog by running `tox run -e changelog-draft`. |
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.
Good idea! I prefer this as well, since it will also help them spot malformed content.
I still think this is handy for maintainers, but we can direct users to tox run -e build-docs
.
-r{toxinidir}/docs/requirements.txt | ||
commands = | ||
towncrier --version | ||
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"], stderr=subprocess.PIPE)' |
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.
You had done something similar:
https://github.com/cherrypy/cheroot/blob/fdaa24d8470d68341a84d977f32436e1344e8230/tox.ini#L336-L346
You used sh
, and I slightly prefer this subprocess usage but don't feel strongly.
At first, I tried without this, thinking it wasn't strictly necessary. But running several times, I found that stderr sometimes was interleaved with stdout. I definitely should add a comment on it though.
If we're wrapping it, I feel like {posargs}
gets a little messy. I guess it could be done with...
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"], stderr=subprocess.PIPE)' | |
{envpython} -c 'import sys, subprocess; subprocess.run([sys.executable, "-m", "towncrier", "build", "--version", "main", "--draft"] + sys.argv[1:], stderr=subprocess.PIPE)' {posargs} |
that? I think that works. I'll try it.
@sirosen my bot for change note fragments is now plugged into this repo and reports via Checks API as follows: https://github.com/jazzband/pip-tools/pull/2203/checks?check_run_id=46466229160. As you get to adding the config (possibly in a separate PR), the reported check name may change (pip has that). |
Haha! I'm getting flashbacks of one time I've gone deep down the rabbit hole to do something similar post Towncrier reconfig in setuptools: pypa/setuptools#2436. I'm not sure how I feel about “Thanks” vs. “by”. This is something a contributor would be adding, so some may feel weird thanking themselves, while regular bylines might not trigger the same reaction. I've been following the style over the past few years, where a byline would be either a part of a sentence (with no period before it but one after), or as a dedicated line (no periods). The standalone byline can be placed either at the end, or after the first paragraph. The in-sentence one could be integrated in one of the sentences wherever it feels right. So my style would be like one of these: A phrase -- by :user:`gh-username`. A phrase -- by :user:`gh-username`.
Another paragraph. A phrase -- by :user:`gh-username`. Extra detail.
Another paragraph. A phrase. More detail.
-- by :user:`gh-username`
Another paragraph.
Or more.
.. code-block:: python
<usage-example>
Recommendations. A phrase. More detail.
Another paragraph.
-- by :user:`gh-username` And with that, references would always come after since they're outside the fragment, in the template + have labels that wouldn't work before the note.
I just know that many people dislike having many things in the root, so I've been moving the fragments deeper (and closer to the docs). But we can keep it at the top if you think it's best. I won't insist. |
These changes are broken down into several commits for simpler review.
What
How
Contributor checklist
changelog.d/
(seechangelog.d/README.md
for instructions) or the PR text says "no changelog needed".Maintainer checklist
skip-changelog
label.