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

I'm worried towncrier raises the threshold for new contributors and doesn't make our changelog better – thoughts? #12337

Closed
hoechenberger opened this issue Jan 5, 2024 · 16 comments

Comments

@hoechenberger
Copy link
Member

hoechenberger commented Jan 5, 2024

Hello, this is no call to change or revert anything right now, but I just wanted to voice my concern that I believe with the adoption of towncrier, contributing has all but become easier … It's at least one additional file a new contributor has to add to git (instead of editing an existing changelog) … And for now, I don't see how it has improved the quality of our changelog entries either – wasn't that the original concern we were trying to address?

No further strong arguments, no further suggestions – just wanted to share my 2 ct and hear your opinions.

Thanks!

Richard

@mscheltienne
Copy link
Member

I haven't followed what towncrier does, but isn't the idea to pull the changelog entries from the github Labels and PR titles and authors?

@hoechenberger
Copy link
Member Author

I haven't followed what towncrier does, but isn't the idea to pull the changelog entries from the github Labels and PR titles and authors?

No, to my knowledge this is not the case. You have to manually create individual files for each changelog section you want to add entries to.

@drammock
Copy link
Member

drammock commented Jan 5, 2024

thanks for the feedback @hoechenberger. For me it's too early to tell (I've been chipping away at one big PR so I haven't actually gone through the towncrier process yet!). Are there PRs you can point to where a new/infrequent contributor was confused or struggled with the changelog step?

One idea for streamlining this is a new GH action that:

  1. checks for one of a set of defined PR labels (bug, enhancement, etc) and comments on the issue requesting a label if one isn't found (or if two are found, asking to remove one of them?)
  2. automatically creates an empty file in the right spot with the right name (unless it's already there in the PR), e.g. doc/changes/devel/12345.bugfix.rst.
  3. comments on the PR prompting the user to add their changelog entry to that newly-created file (it could even offer a direct link to the GH edit interface for that file)

@cbrnr
Copy link
Contributor

cbrnr commented Jan 5, 2024

I already submitted two PRs with the new changelog workflow. I think the process is more difficult now. Previously, we could add changelog entries to one file in the most suitable section. Now we need to create a new file with the correct scheme (the name must contain the PR number and the type of change).

I really don't know what the benefit of the new workflow is. MNE already had a good practice of keeping a user-centric changelog, and AFAIR we wanted to improve the messages themselves to make them even easier for users. The goal was not to introduce yet another tool and make things more complicated.

Tiny changes don't need a changelog entry, and now this means that the check will fail in such a case. This is also not ideal in terms of ease of use.

So I think that the current situation is a regression in terms of usability. I also advocate for keeping things simple. As I've mentioned in e.g. #12051, I'd keep our previous process of manually adding changes to a single file. And if we want to catch PRs that don't include a changelog entry, we can do this with a simple GitHub Action.

@drammock
Copy link
Member

drammock commented Jan 5, 2024

Now we need to create a new file with the correct scheme (the name must contain the PR number and the type of change)

I believe that if the file is just called bugfix.rst then the towncrier action will auto-add the PR number to the filename. At least that was the intended behavior; maybe it doesn't work that way yet?

AFAIR we wanted to improve the messages themselves to make them even easier for users.

I thought the main goal was reducing the frequency of changelog entries being forgotten and/or maintainers having to request them from PR submitters. Making the entries better or easier for PR submitters to write would be a nice benefit but I didn't expect towncrier to be able to do that... there was some talk of doing additional inspection of the changelog entries (similar to what's often done with docstrings, e.g., "must start with capital and end with period; must start with a verb and use active voice; etc") which might yield a net improvement in the quality of the entries, but hasn't been tried.

Tiny changes don't need a changelog entry, and now this means that the check will fail in such a case. This is also not ideal in terms of ease of use.

yes, but as you saw in #12335 adding the no-changelog-entry-needed label handles that case (I think even retroactively? the one and only commit in that PR is all green, even though the label was added later). There is still room for improvement though; a bot-generated comment on the PR saying "this needs a changelog label ('bug', 'enhancement', etc) --- use the label no-changelog-entry-needed if that's appropriate" could avert the confusion I'd think.

@mscheltienne
Copy link
Member

mscheltienne commented Jan 5, 2024

I believe that if the file is just called bugfix.rst then the towncrier action will auto-add the PR number to the filename. At least that was the intended behavior; maybe it doesn't work that way yet?

I don't think so, at least not according to https://mne.tools/dev/development/contributing.html#describe-your-changes-in-the-changelog

I thought the main goal was reducing the frequency of changelog entries being forgotten and/or maintainers having to request them from PR submitters.

After my first PR using towncrier, I agree that it will make it worst. Previously, you could link the changelog file and request an entry under the correct section based on the 'templates' found in the same file. Now, you have to request a new file, with the correct name, and with the correct content. And now, the 'templates' are in the same directory instead of the same file.

Tiny changes don't need a changelog entry, and now this means that the check will fail in such a case. This is also not ideal in terms of ease of use.

For that one, the no-changelog-entry-needed label works great.


IMO, towncrier might simplify the release process, but it does raise the contributing barrier. I'm already seeing discrepancies in the changelog entries on main: capital letters, syntax, author syntax at the end of the sentence, presence or absence of the :gh: role on top of the PR number in the file name.. how is a contributor suppose to easily figure out which entry to take as template?

I would prefer a tool based on PR names, authors and labels; in which case the changelog completeness falls under the responsability of maintainers and is entirely transparent to users/contributors.

@hoechenberger
Copy link
Member Author

hoechenberger commented Jan 5, 2024

@drammock

Are there PRs you can point to where a new/infrequent contributor was confused or struggled with the changelog step?

No, but I believe we added more complexity to the process of committing a changelog entry. The volume of our contributing guide IMHO is already totally out of hand, and the section for adding changelogs is 3 2.5 screens long on my laptop computer. Whom can we even expect to read and comprehend all of that? I would never, ever, read all this (unless somebody forced me to 🥲). I'd rather not contribute in the first place.

I believe @mscheltienne and @cbrnr basically made all points that I think are relevant. Right now, I don't see how towncrier streamlines our changelog process in any way, nor how it makes changelogs more user-friendly. Instead, we added loads of new tooling and more complexity. Am I missing some integral parts here? Why do we even need towncrier? The only advantage it currently brings is that it avoids merge conflicts in the changelog, no? – It's an honest question, I really don't see any other advantages…?

@larsoner
Copy link
Member

larsoner commented Jan 8, 2024

I believe that if the file is just called bugfix.rst then the towncrier action will auto-add the PR number to the filename. At least that was the intended behavior; maybe it doesn't work that way yet?

I don't think so, at least not according to https://mne.tools/dev/development/contributing.html#describe-your-changes-in-the-changelog

It will, I added an magical little action for it in #12318, though perhaps I didn't document it enough. If you push a PR with doc/changes/devel/bugfix.rst it will git mv it based on the PR number for you.

IMO, towncrier might simplify the release process, but it does raise the contributing barrier.

I think the more we conform to scientific Python common community practices the better. Towncrier-style changes are used by numpy, matplotlib, etc. so I think from an ecosystem perspective you could argue that anyone familiarizing themselves with Towncrier will automatically benefit from "getting it" for the others, so it lowers the overall long-term burden and has pedagogical value.

The volume of our contributing guide IMHO is already totally out of hand... Why do we even need towncrier? The only advantage it currently brings is that it avoids merge conflicts in the changelog, no? – It's an honest question, I really don't see any other advantages…?

If you read @drammock's comment parts are in there:

reducing the frequency of changelog entries being forgotten and/or maintainers having to request them from PR submitters

Just to suppliment this a bit, historically changelog.rst has been the source of the vast majority of merge conflicts, which maintainers or contributors have to deal with. We also forget entries for PRs we want them for maybe 5-10% of the time. Those problems are now solved. And as far as the contributing guide goes, I think the guidelines actually got shorter in #12299 though it's harder to tell because content was consolidated across files. But given the new structure I don't think it's really too hard to guess the right thing to do based on what's there already.

I would prefer a tool based on PR names, authors and labels; in which case the changelog completeness falls under the responsability of maintainers and is entirely transparent to users/contributors.

Given infinite time to work on these sorts of things I agree manual maintainer-based curation would be best. But given limited maintainer time (possibly better spent elsewhere) I think it's reasonable to offload some of this work to contributors.

Overall I think it's worth trying this for a while and seeing if new contributors actually do have issues with it. I think we have some evidence from #12319 that it was not too onerous for at least one new contributor. I think existing MNE contributors will also trip over it initially but hopefully it will make things easier. FWIW with the auto-renaming I expect it to make things easier for my PRs as a contributor.

@cbrnr
Copy link
Contributor

cbrnr commented Jan 8, 2024

I still maintain that a simple GHA (e.g. this one or this one) could check if the PR contains a changelog entry (and if not fail). Merge conflicts are not that common, and if they occur, these are the easiest merge conflicts that every developer should be able to handle (or in other words, a nice opportunity to learn here).

@cbrnr
Copy link
Contributor

cbrnr commented Jan 8, 2024

But yes, the new workflow automatically fills in the PR number, so that's definitely an improvement (but it is not documented yet).

@mscheltienne
Copy link
Member

Good arguments, I agree that merge conflicts are easy to solve, but not for all contributors, thus limitting this common merge conflicts is an improvement.

@hoechenberger
Copy link
Member Author

I'm not convinced, my perception is we're exchanging some minor inconveniences for new problems and more tooling without making anything actually better for anyone. Notably, the core issue – better changelogs for users – was not even touched upon. I find this a very frustrating experience, and I'm also frustrated about the way this change came about, i.e., the decision process leaves me feeling alienated. I don't have the energy to keep discussing this, so I'm closing this issue.

@hoechenberger hoechenberger closed this as not planned Won't fix, can't repro, duplicate, stale Jan 8, 2024
@drammock
Copy link
Member

drammock commented Jan 8, 2024

I'm not convinced, my perception is we're exchanging some minor inconveniences for new problems and more tooling without making anything actually better for anyone.

That is valuable feedback, thanks.

Notably, the core issue – better changelogs for users – was not even touched upon.

I don't think that's a very fair characterization of the discussion; it was touched upon. Your original post asked "wasn't [the quality of changelog entries] the original concern we were trying to address?" and, as I responded here, the motivation for adopting towncrier was "reducing the frequency of changelog entries being forgotten and/or maintainers having to request them from PR submitters". That comment also explained that there had been discussions of other things we could try to maybe improve changelog quality, but they haven't yet been tried. That doesn't mean that improving quality is unimportant, nor does it mean that making the process easier for contribs is unimportant. But they are separate goals with potentially separate solutions. (Incidentally, I also offered some additional ideas for lowering the contrib barrier in this comment --- which neither you nor anyone else has engaged with / responded to).

I find this a very frustrating experience, and I'm also frustrated about the way this change came about, i.e., the decision process leaves me feeling alienated. I don't have the energy to keep discussing this, so I'm closing this issue.

I'm sad to hear that. As I've said before, our use of towncrier is meant to be an experiment (not a fait accompli) and I'm glad you got the ball rolling on giving feedback. I also think it's too early to decide for/against it (@larsoner has similarly said "Overall I think it's worth trying this for a while") so I'm asking for you to be patient while we gather more opinions / experience.

@drammock drammock reopened this Jan 8, 2024
@hoechenberger
Copy link
Member Author

hoechenberger commented Feb 29, 2024

To me this discussion is mostly closed – towncrier doesn't seem to actually add a barrier as I first suspected; seems I was wrong.

It still doesn't sit quite right with me – so much tooling, no better changelogs still. But oh well, so be it.

But I wanted to share something I just came across, which I'd prefer very much to towncrier, should we ever consider ditching it again:

mamba-org/mamba#3179

Basically, they use the gh CLI to extract all changelog-relevant info from PRs with a specific label, and use this to compile the changelog.

Try it out yourself:

  • Install gh
  • Navigate into your local mne-python directory
  • Run:
    gh repo set-default
    gh pr view 12450
    gh pr view 12450 --json title
    gh pr view 12450 --json author
    gh pr view 12450 --json body

… you get the idea…

@drammock
Copy link
Member

To me this discussion is mostly closed – towncrier doesn't seem to actually add a barrier as I first suspected; seems I was wrong.

Thanks for sharing your updated opinion, good to know.

It still doesn't sit quite right with me – so much tooling, no better changelogs still

I'm sympathetic with this view. I don't think there's zero benefit (PRs accidentally missing changelog entries are much less likely because they cause a failing CI now, and changelog entries can't accidentally reference e.g. the issue number instead of the PR number). I guess some additional gains may come at release time?

But I wanted to share something I just came across

That is reminiscent of the home-grown idea from #12051 (comment) which is something both @hoechenberger and I were in favor of exploring. Using gh under the hood looks like a nice expedient. For me that approach is the clear front-runner for "next thing to try after towncrier" but I feel I should defer to @larsoner on if/when we do try something else (because he is usually the one pushing out our releases and probably handles more PR reviews than anyone else, and thus is most benefitted by any towncrier-related gains, however small they be). If it comes to a vote, I'll vote however he does.

@hoechenberger
Copy link
Member Author

hoechenberger commented Apr 26, 2024

Hello, with the new autofix.ci feature I really like the new workflow for me as a developer! Again, I have to admit I was wrong and too pessimistic.

I'm still not super convinced about the quality of our changelog entries, but the workflow for adding them is nice now (unless I'm adding one of the very first entries after a release, where I always have go and find an older template to assist me, as the respective folder gets cleared with a release).

All in all, and ignoring the additional tooling required (which still makes me a little anxious!), I appreciate this new way of working and I'd like to say thanks to all who helped implement this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants