fix: More robust version check #104
Conversation
M123-dev
commented
May 25, 2023
M123-dev
commented
- Fixes: Smarter changelog version extraction #103
- Now checking if the changelog header is containing the version not if they equal, allowing for more custom header (e,g, using release-please)
- I’ve reviewed the contributor guide and applied the relevant portions to this PR.
|
"combust" version check? 😄 |
pkgs/firehose/lib/firehose.dart
Outdated
| @@ -130,7 +130,7 @@ Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automati | |||
| print('changelog:'); | |||
| print(package.changelog.describeLatestChanges.trimRight()); | |||
|
|
|||
| if (pubspecVersion != changelogVersion) { | |||
| if (!changelogVersion.toString().contains(pubspecVersion)) { | |||
There was a problem hiding this comment.
This will allow correct changes (1.2.3 and [1.2.3](...)) but will also allow incorrect ones (foo 1.2.3 bar).
We should be more precise w/ this check. I think that either means updating Changelog.latestVersion to handle the two supported types of version formats, or changing Changelog.latestVersion to be named Changelog.lastHeader and having code which can understand that as either a version or a linked version.
We should also have a test of the code that can parse [1.2.3](...) => 1.2.3.
There was a problem hiding this comment.
No problem, I can add some test, but regarding the way to extract the number. Why would foo 1.2.3 bar wrong? Some repos meight use Version: 1.2.3 [date] or something similar as header.
The only way this contains could go wrong is this dates. When the version is 26.5.2023.
I'd recommend adding a regex match [0-9]+.[0-9]+.[0-9]+ as Changelog.latestVersion.
We could allow to change from the yaml it but I don't think that is neccesary.
What do you think?
There was a problem hiding this comment.
Why would foo 1.2.3 bar wrong? Some repos meight use Version: 1.2.3 [date] or something similar as header.
I think I'd prefer to be proscriptive here - there's no hard format for a changelog; I'd just like to support a few clear, simple formats.
If you think you can pull the version out w/ a regex reliably (and have a few positive and negative tests to show that), sgtm.
There was a problem hiding this comment.
I've switched to using regex and added a lot of test cases, please let me know if that is what you had in mind
devoncarew
added
status-needs-info
needs-info
Ohhh, I was in a meeting while commiting should of course be "robust" don't know what hit me there 😆 |
M123-dev
changed the title
github-actions
bot
removed
the
needs-info
Reverted logic in firehose.dart Updated latestVersion Added a new latestHeading method Added lots of test cases
|
Thanks a lot @devoncarew, I've applied your comments and bumped the version. |
