fix: detect categories with an explanation mark indicating breaking changes #2132
Conversation
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
I think there's a bug. The approach looks good overall though.
Would it be possible to see the output for an example release (e.g. this planned one) with both versions before merging?
Oops, yes. I broke things moving some stuff around just before committing.
Yeah, I was being a bit lazy thinking I would test it with the release for today rather than go to the effort of organising a separate release. But that would be better I guess. Will do in a moment after addressing the other comments. |
benhoyt
left a comment
benhoyt
left a comment
There was a problem hiding this comment.
I'm happy to just go with James' review on this one, seeing it's the release script. Thanks!
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Looks good to me. I'm happy for this to be tested by trying to release the beta if you are.
I guess we're not type checking this script, because it should complain that we're passing a Mapping (categories) to functions that expect a dict (format_changes, format_release_notes) -- probably worth fixing before merging just for correctness, but shouldn't have any effect at runtime.
| if not re.match(r'^\d+\.\d+\.\d+$', new_tag): | ||
| logger.error('Error: Tag must be in format X.Y.Z') | ||
| if not re.match(r'^\d+\.\d+\.\d+(?:(?:a|b|rc)\d+)?$', new_tag): | ||
| logger.error('Tag must be in format X.Y.Z') |
There was a problem hiding this comment.
I guess this doesn't handle pre and post releases either. Perhaps saner to parse with packing.version, but probably fine for now.
There was a problem hiding this comment.
Yeah, since this wasn't the main point of the PR and I just needed it to get the script to run, I did the minimal change. I agree using the official parser would be better, but I think that's a separate PR and more thought should be put into how the pieces are used.
|
Release notes look like this: 
Changelog looks like this: 
|
3 participants
Currently, the release script fails to recognise lines with the
!breaking change indicator in the category, meaning they are not included in the change log or release notes (and yet, are probably the most important to include).The PR:
!if present.Also, to handle the upcoming release, adds support for alpha, beta, and release candidate numbers in the version (in a fairly minimal way - the tag, the title, and generally not crashing).
Fixes #2111