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

improve handling of changelog processing for backports #2041

Merged
merged 6 commits into from Dec 18, 2023

Conversation

nickfyson
Copy link
Contributor

@nickfyson nickfyson commented Dec 15, 2023

This PR fixes handling of backporting the changelog when the only listed change for a version is not applicable for the older targeted release.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@nickfyson nickfyson force-pushed the nickfyson/fix-changelog-backports branch from 4448494 to f87484e Compare December 15, 2023 16:45
@nickfyson nickfyson marked this pull request as ready for review December 15, 2023 16:47
@nickfyson nickfyson requested a review from a team as a code owner December 15, 2023 16:47
@nickfyson nickfyson force-pushed the nickfyson/fix-changelog-backports branch from f87484e to 8e4a6c7 Compare December 15, 2023 16:50
henrymercer
henrymercer previously approved these changes Dec 15, 2023
Copy link
Contributor

@henrymercer henrymercer left a comment

Choose a reason for hiding this comment

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

Looks good. Another approach would be parsing the document in full into a version -> changelog notes map, performing our tweaks on the map, then rewriting to Markdown. I think that might be more readable, but it's probably not better enough to justify changing it — what do you think?

.github/update-release-branch.py Outdated Show resolved Hide resolved
.github/update-release-branch.py Outdated Show resolved Hide resolved
.github/update-release-branch.py Outdated Show resolved Hide resolved
nickfyson and others added 2 commits December 15, 2023 18:44
Co-authored-by: Henry Mercer <henry.mercer@me.com>
henrymercer
henrymercer previously approved these changes Dec 15, 2023
@nickfyson
Copy link
Contributor Author

Yeah, I started doing that and then decided it was overkill. But then had to work pretty hard to make this approach even vaguely grokkable. 😅

I think I'll leave it with this approach, and if/when it gets more complicated then refactor it 'properly'. 😄

aeisenberg
aeisenberg previously approved these changes Dec 15, 2023
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. Two non-blocking suggestions.

Comment on lines 190 to 192
line = f.readline().rstrip()

output += line + '\n'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is minor and non-blocking, but why rstrip and then add \n? If the original changelog has trailing whitespace (even though it shouldn't), then would it be best to keep the new one as close to the original as possible?

line = f.readline().rstrip()

output += line + '\n'
if line.startswith('## '):
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't have to handle this, but this script will fail if there is no ## line. Maybe just to be safe include a fallback to handle this case (I could imagine this might happen if we're testing something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added an explicit error message in this situation, to try and avoid confusion should it arise. 👍

@nickfyson nickfyson force-pushed the nickfyson/fix-changelog-backports branch from 9e6fc4c to 950c51d Compare December 18, 2023 10:28
@nickfyson nickfyson merged commit 0978396 into main Dec 18, 2023
339 checks passed
@nickfyson nickfyson deleted the nickfyson/fix-changelog-backports branch December 18, 2023 19:23
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.

None yet

3 participants