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.

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

Adds towncrier functionality to help improve automation of release note aggregation. #5477

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

leej3
Copy link
Contributor

@leej3 leej3 commented Jul 16, 2021

One additional bit of functionality that we considered adding but have not yet implemented is to add some automation for checking if every PR has news fragments. Or perhaps a bot to remind people to add it… or auto-commit an example news fragment that contributors can modify. Let us know if this sounds like something worth doing. The quickest and easiest alternative would be to just add the following to the PR template:

The release notes are managed using
[towncrier](https://pypi.org/project/towncrier/) and all non trivial changes
must be accompanied by a [news fragment
entry](https://github.com/twisted/towncrier#news-fragments). Fragment file
name should be “doc/source/upcoming_changes/<PR number>.<type>.rst” where
"type" can be one of  api, improvements, deprecated, process, feature, bugfix,
doc, (or trivial if you do not wish the changes included in the release notes.

Example: 5456.bugfix.rst

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.

This adds a github action to run towncrier and some custom python
functionality to aggregate information to document changes for the
current release.
@pep8speaks
Copy link

pep8speaks commented Jul 16, 2021

Hello @leej3! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-11-23 01:28:38 UTC

@grlee77 grlee77 added the 🤖 type: Infrastructure CI, packaging, tools and automation label Jul 16, 2021
@leej3 leej3 changed the title Adds towncrier functionality to help improve automation of release note aggregation. WIP:Adds towncrier functionality to help improve automation of release note aggregation. Jul 16, 2021
@leej3
Copy link
Contributor Author

leej3 commented Jul 16, 2021

Will fix tests/code style next week.

@mkcor
Copy link
Member

mkcor commented Jul 17, 2021

Love it! Thanks, @leej3. This PR submission is giving the extra motivation I need to review #5463 😉

```
cd <path/to/scikit-image/repository>
pip install -r requirements/_release_docs.txt
python tools/aggregate_contributors <path/to/scikit-image>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python tools/aggregate_contributors <path/to/scikit-image>
python tools/aggregate_contributors .

Using <path/to/scikit-image> her and <path/to/scikit-image/repository> above can potentially confuse readers into thinking these are two different paths

we are already in the repository path due to the command above, so I just switched to . here

Comment on lines 20 to 21
- {{ text }} {% if category != 'process' %} {{ values|sort|join(', ') }} {% endif %} {% endfor %}
{% else %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- {{ text }} {% if category != 'process' %} {{ values|sort|join(', ') }} {% endif %} {% endfor %}
{% else %}
- {{ text }} {% if category != 'process' %} {{ values|sort|join(', ') }} {% endif %} {% endfor %}
{% else %}

Seems to need this blank line here, otherwise I get the following in a case I tested with two doc and one bugfix entry (Note the missing blank line before the start of the "Documentation" section)

Bug Fixes
---------
 
- Minor fixups to Hough line transform code and examples  `#5182 <https://github.com/scikit-image/scikit-image/issues/5182>`_    
Documentation
-------------
 
- Prevent integer overflow in EllipseModel  `#5179 <https://github.com/scikit-image/scikit-image/issues/5179>`_   
- update reference link in exposure module  `#5473 <https://github.com/scikit-image/scikit-image/issues/5473>`_


- name: Get reviewers and authors
run: |
python tools/aggregate_contributors.py ${{ github.workspace }}
Copy link
Contributor

Choose a reason for hiding this comment

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

It is nice that this script also credits reviewers!

@grlee77
Copy link
Contributor

grlee77 commented Jul 19, 2021

I like the idea of adding the instructions for this to the PR template. Please also update the section labeled 5. Document changes in CONTRIBUTING.TXT.

For this to be useful we do need to remember to always add a news fragment before merging PRs, so that is one extra check that needs to be done by maintainers prior to merge. I'm not sure whether we should automatically block merging somehow until a fragment is present?

@viniciusdc
Copy link
Contributor

I like the idea of adding the instructions for this to the PR template. Please also update the section labeled 5. Document changes in CONTRIBUTING.TXT.

For this to be useful we do need to remember to always add a news fragment before merging PRs, so that is one extra check that needs to be done by maintainers prior to merge. I'm not sure whether we should automatically block merging somehow until a fragment is present?

Hi @grlee77, that was one of the ideas we've been discussing so far. We thought of adding a linter check for the news fragment files or perhaps an auto generation tool to run once merged... Would love to listen your thoughts about that

@grlee77
Copy link
Contributor

grlee77 commented Jul 19, 2021

It might be checking in with NumPy devs how it has been working out for them. Also, we should consider harmonizing the category names with what NumPy is using

I am pretty sure NumPy is not enforcing that a news item be in all PRs though, because I recently made a small bug fix PR there and was not asked to add one.

@grlee77
Copy link
Contributor

grlee77 commented Jul 19, 2021

We thought of adding a linter check for the news fragment files or perhaps an auto generation tool to run once merged... Would love to listen your thoughts about that

How would you determine the category if autogenerating it? If we consistently labeled every PR, the labels could be used, but we sometimes miss adding them.

I would be curious to hear from other contributors about how helpful vs. annoying having a linter option would be. It seems helpful to the release manager at cost of being mildly annoying to contributors.

@viniciusdc
Copy link
Contributor

viniciusdc commented Jul 19, 2021

It might be checking in with NumPy devs how it has been working out for them. Also, we should consider harmonizing the category names with what NumPy is using

I am pretty sure NumPy is not enforcing that a news item be in all PRs though, because I recently made a small bug fix PR there and was not asked to add one.

Definitely! For the categories I just followed what I found under the sections of some old scikit-image release files. To change then or adding new ones, we just need to modify the pyproject.toml file

@leej3
Copy link
Contributor Author

leej3 commented Jul 20, 2021

Thanks for the feedback @grlee77.

How would you determine the category if autogenerating it?

We wouldn't be able and would have to choose a default. This is a flaw in the approach that likely makes it untenable (unless a set of labels for this start to get used religously).

I would be curious to hear from other contributors about how helpful vs. annoying having a linter option would be.

Agreed. We would like feedback on this. A linter/bot to
remind the contributor (but not block the merge) seems
like the most gentle approach that would encourage
people to add this but not enforce it. It is easy
enough and would only take about a day. Prior to
release, the core maintainer would do a check that the
PRs have an associated fragment or fragments. FWIW
I think this would be my preference if labels are not
being used consistently. I'm not a maintainer though!

It might be checking in with NumPy devs how it has been working out for them. Also, we should consider harmonizing the category names with what NumPy is using

Yes, checking in with them sounds like a good idea. The current sections listed in the pyproject.toml can be easily modified and were chosen based on past releases and the release template. This history speaks to the usefulness of those sections for this project but considering other approaches at this point is also sensible. Numpy seem to have a focus on deprecation/compatibility etc. that the current sections do not address. Maybe others could comment on the proposed release notes sections?

We have a few things we still wish to add:

  • Only push back news fragment deletion for releases (not release candidates)
  • Add a news fragment for this PR
  • Move towncrier.md into upcoming_changes as numpy does (better place for it)
  • Modify section 5. of CONTRIBUTING.txt
  • Modify PR template

Details that we will wait for more feedback on:

  • Altering release notes section headings according to a standard or otherwise (redefine the fragment types)
  • Implement a check for fragment addition in the form of blocking a merge, reminding via bot, automate a somewhat templated one that can serve as a placeholder/cue for memory.

@leej3 leej3 changed the title WIP:Adds towncrier functionality to help improve automation of release note aggregation. Adds towncrier functionality to help improve automation of release note aggregation. Jul 21, 2021
@grlee77
Copy link
Contributor

grlee77 commented Nov 23, 2021

@scikit-image/core, now that we have branched v0.19.x, it would be a good time to consider merging this to main and starting to add the news fragments to new PRs so that we can use this approach to manage the release notes for the next release.

If it ends up not being helpful for some reason, we can always stop using it and go back to manual release note updates or evaluate other alternatives.

@stefanv
Copy link
Member

stefanv commented Nov 23, 2021

I would like us to consider one other option. Instead of using towncrier this way, can we write a script that parses PR descriptions like the following:

```release-note
Add a faster implementation of the foo-detector.
```

We could also consider having a category tag:

```release-note: bug
Fixes that annoying bug that's been with us since skimage v0.16.
```

This way, we can keep descriptions with the PRs, and easily extract them, format them into the release notes, etc.

@grlee77
Copy link
Contributor

grlee77 commented Nov 23, 2021

I will try to fix the conflicts here now

@grlee77
Copy link
Contributor

grlee77 commented Nov 23, 2021

@stefanv, if I understand correctly, you are proposing extracting everything from the PR descriptions and we would not add news fragment files at all? (i.e. the script would autogenerate the fragment files from the release notes so we can still use towncrier?).

@stefanv
Copy link
Member

stefanv commented Nov 23, 2021

Yes, or we can put it in a Markdown file, e.g, then we don't need the added complexity of towncrier. Or we use that information as source for towncrier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 type: Infrastructure CI, packaging, tools and automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants