-
Notifications
You must be signed in to change notification settings - Fork 348
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
Conversation
4448494
to
f87484e
Compare
f87484e
to
8e4a6c7
Compare
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.
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?
Co-authored-by: Henry Mercer <[email protected]>
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'. 😄 |
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.
Looks reasonable to me. Two non-blocking suggestions.
.github/update-release-branch.py
Outdated
line = f.readline().rstrip() | ||
|
||
output += line + '\n' |
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 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('## '): |
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 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).
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've added an explicit error message in this situation, to try and avoid confusion should it arise. 👍
9e6fc4c
9e6fc4c
to
950c51d
Compare
950c51d
to
8e086df
Compare
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