Skip to content

Move unreleased changelog entries to CHANGELOG.d#4132

Merged
rhendric merged 2 commits intopurescript:masterfrom
rhendric:rhendric/changelog-dir
Jul 12, 2021
Merged

Move unreleased changelog entries to CHANGELOG.d#4132
rhendric merged 2 commits intopurescript:masterfrom
rhendric:rhendric/changelog-dir

Conversation

@rhendric
Copy link
Copy Markdown
Member

@rhendric rhendric commented Jul 3, 2021

Description of the change

Instead of editing CHANGELOG.md, let's create files in CHANGELOG.d. CHANGELOG.d/README.md explains the scheme, and update-changelog.hs does the hard work at release time.

If you'd like to play around with the release-time ergonomics, get a test branch with git clone -b rhendric/test-for-changelog-dir --single-branch https://github.com/rhendric/purescript.git, run ./update-changelog.hs, and see what happened with git diff --cached.

Closes #4130.


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@rhendric rhendric force-pushed the rhendric/changelog-dir branch 2 times, most recently from 1ed9a95 to a6749b8 Compare July 3, 2021 02:22
@JordanMartinez
Copy link
Copy Markdown
Contributor

Just a few thoughts

  • does this script also add a new [Unreleased] section? It looks like it doesn't, but I might have missed it
  • while the automatic PR number and author finder is nice, part of me still thinks that should be done manually. When a feature spans multiple PRs, we would want to edit the same entry multiple times as each part of it is merged (e.g. kind signatures in docs) rather than having multiple entries. When such entries are edited, then this code would include that author as well despite them not having contributed to the code, right?

Other than that, it looks good.

@rhendric
Copy link
Copy Markdown
Member Author

rhendric commented Jul 3, 2021

  • does this script also add a new [Unreleased] section?

No, the whole idea is to get rid of that! This PR can't actually delete that section yet because I didn't want to mess with your release PR, but the idea is that CHANGELOG.md contains released changelog entries, and CHANGELOG.d contains unreleased entries.

  • When a feature spans multiple PRs, we would want to edit the same entry multiple times as each part of it is merged (e.g. kind signatures in docs) rather than having multiple entries. When such entries are edited, then this code would include that author as well despite them not having contributed to the code, right?

Yeah, so this is a little complex. The design (which could be imperfectly implemented) is as follows:

  • All commits that touch an entry file, or are a merge commit in the path from HEAD to such a commit, are considered relevant to that entry.
  • Any relevant commits that are not GitHub squash or merge commits are then discarded.
  • Of what remains, any squash commits that only touch changelog entries are discarded. (This is intended to filter out commits that reword a previous entry but don't contribute any code. This check also includes CHANGELOG.md itself, in case a larger reword is happening that encompasses both past and present changelog entries.)
  • PR numbers are collected from the remaining relevant commits, and their authors are resolved and nubbed.

So if a feature spans multiple PRs, any PR that edits the same entry file and also touches some non-changelog file in the repo gets added to the list for that entry. Any PR that only edits entry files is not mentioned.

@JordanMartinez
Copy link
Copy Markdown
Contributor

No, the whole idea is to get rid of that!

🤦‍♂️ Ha! I'm dumb.

Well great! I like this idea a lot! Well done.

@rhendric rhendric force-pushed the rhendric/changelog-dir branch from a6749b8 to 59de313 Compare July 6, 2021 20:11
@rhendric
Copy link
Copy Markdown
Member Author

rhendric commented Jul 6, 2021

Updated to actually remove the Unreleased section from the changelog, now that it's empty.

@rhendric
Copy link
Copy Markdown
Member Author

rhendric commented Jul 6, 2021

I try not to be pushy about my PRs, but I think it would be particularly convenient for this to be reviewed and, if acceptable, merged before any other PRs, now that 0.14.3 is released and there aren't any unreleased changelog entries.

@JordanMartinez
Copy link
Copy Markdown
Contributor

I'm in full agreement of getting this merged first before any other PRs.

@JordanMartinez
Copy link
Copy Markdown
Contributor

I checked this out locally to see how this works. @rhendric Thanks for making me laugh. The changelog was a fun read.

Running stack update-changelog.hs took a few minutes to build initially. AFAICT, this long initial build is the only downside of this script. What if a developer wanted to verify that their entry would display correctly in the resulting changelog? Running this to verify that it works as expected wouldn't be a "few milliseconds" tests by running that command.

Once it did build, rerunning it took about 4 seconds.

When I did build the changelog via the above command (after updating the version in npm-package/json), the CHANGELOG.md file was updated to this. I probably don't need to say this, but this is the Internet: the below summaries, linked PRs, and their supposed authors are all fake data:

## 0.14.4

New features:

* Support cobras to control anteater population (#4123 and #4100 by @varikvalefor and @JordanMartinez)

  The cobras are themselves controlled by a mongoose.

* Add a doberman to guard the big pit (#4129 by @rhendric)

Bugfixes:

* Use anteaters to clean up rooms (#4096 by @rhendric)

Internal:

* Move unreleased changelog entries to CHANGELOG.d

  See CHANGELOG.d/README.md for details.

* Use a big pit to hold all these animals (#4113 by @f-f)

At some point, as I was playing around with it, I modified one of the files in the CHANGELOG.d folder but didn't commit it. Running stack update-changelog.hs produced this error:

error: the following file has local modifications:
    CHANGELOG.d/fix_stuff.md
(use --cached to keep the file, or -f to force removal)
update-changelog.hs: git rm -q CHANGELOG.d/feature_cobras.md CHANGELOG.d/feature_doberman.md CHANGELOG.d/fix_anteaters.md CHANGELOG.d/fix_stuff.md CHANGELOG.d/internal_changelog-dir.md CHANGELOG.d/internal_big-pit.md failed with exit code 1

I also intentionally created a file with a lot of blank lines at the end of it to see what would happen. Why? My editor always adds a blank newline to the bottom of a file, whether I need it or not:

* fix some stuff



  and some more stuff


  

This changelog script did not trim the three extra lines at the bottom of the file. Below is a snippet of what it produced:

Bugfixes:

* Use anteaters to clean up rooms (#4096 by @rhendric)

* fix some stuff



  and some more stuff


  

Internal:

The same extra newlines issue occurs if there are extra newlines above the file's content. For example:




* fix some stuff

Perhaps this line trimming issue should be addressed, but since we can address it when reviewing PRs, I don't think it's absolutely necessary.

If anything, @rhendric, do you think there should be an example_how to add an entry.md file to which a new contributor could refer in case they are contributing their first PR to this repo when the CHANGELOG.d folder is empty because we just had a new release?

@rhendric
Copy link
Copy Markdown
Member Author

Running this to verify that it works as expected wouldn't be a "few milliseconds" tests by running that command.

Sad but true. How often do you think contributors will really want to do this, though? The processing done to the entries is pretty minimal; I expect once the novelty wears off, this script will get run about as often as the license generator, which has a similar startup cost.

I modified one of the files in the CHANGELOG.d folder but didn't commit it.

That's a good test. Failing at this step—with CHANGELOG.md updated and git added but the CHANGELOG.d files not yet deleted—is a little sloppy. Do you think it should force delete, so that whatever was written to the entries locally gets added to the changelog as if it had been committed? Or should it check for local modifications before writing to CHANGELOG.md, and ask the user to commit or revert them first if there are any? Or something else?

Perhaps this line trimming issue should be addressed

I think that's a good idea. I didn't think it mattered much because in Markdown, many line breaks are the same as one, but if you cared enough to look for it, I can care enough to make it correct.

do you think there should be an example_how to add an entry.md file

Doesn't CHANGELOG.d/README.md do that well enough?

@JordanMartinez
Copy link
Copy Markdown
Contributor

Doesn't CHANGELOG.d/README.md do that well enough?

I read it again. Yeah, It does. Never mind.

Sad but true. How often do you think contributors will really want to do this, though?

Probably not that often. Again, I think this is the only downside, but it's a very small downside in comparison to the other advantages this provides.

I didn't think it mattered much because in Markdown, many line breaks are the same as one, but if you cared enough to look for it, I can care enough to make it correct.

I don't think it needs to be changed until we experience this problem enough to warrant the change. Let's keep this tool simple and see how things go.

That's a good test. Failing at this step—with CHANGELOG.md updated and git added but the CHANGELOG.d files not yet deleted—is a little sloppy. Do you think it should force delete, so that whatever was written to the entries locally gets added to the changelog as if it had been committed? Or should it check for local modifications before writing to CHANGELOG.md, and ask the user to commit or revert them first if there are any? Or something else?

Good questions. I think the force-delete would be an unexpected change if I was actually trying to change one of the entries. I think it would be better to force the user to commit them, so that it appears in the diff when we're reviewing the release PR.

@rhendric
Copy link
Copy Markdown
Member Author

@JordanMartinez I had already implemented stripping extra whitespace by the time I read your comment; it's pretty simple.

Feedback is addressed in 9865dd8, and my test branch has been rebased atop this one. If you could give this all one final look-over before I merge?

@JordanMartinez
Copy link
Copy Markdown
Contributor

@rhendric Thanks for doing that. Yeah, this looks good. Feel free to merge!

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

Successfully merging this pull request may close these issues.

Our changelog strategy is a pit trap

3 participants