improve handling of changelog processing for backports - #2041
Conversation
Sorry, something went wrong.
henrymercer
left a comment
There was a problem hiding this comment.
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?
Sorry, something went wrong.
Co-authored-by: Henry Mercer <henry.mercer@me.com>
|
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'. 😄 |
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
Looks reasonable to me. Two non-blocking suggestions.
Sorry, something went wrong.
| line = f.readline().rstrip() | ||
|
|
||
| output += line + '\n' |
There was a problem hiding this comment.
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?
Sorry, something went wrong.
| line = f.readline().rstrip() | ||
|
|
||
| output += line + '\n' | ||
| if line.startswith('## '): |
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
I've added an explicit error message in this situation, to try and avoid confusion should it arise. 👍
Sorry, something went wrong.
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