Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork304
fix semver not fully covered#1548
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
fix semver not fully covered#1548
Uh oh!
There was an error while loading.Please reload this page.
Conversation
0972ba2 to877fcd6Compare| (("1.0.0-reallyweird","PATCH","reallyweird",0,None),"1.0.0-reallyweird1"), | ||
| (("v0.7.1-release","PATCH","release",0,None),"0.7.1-release1"), | ||
| (("v0.0.1-SNAPSHOT","PATCH","SNAPSHOT",0,None),"0.0.1-snapshot1"), |
bearomorphismJun 12, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
@Lee-W I'm not entirely sure if these test cases are correct. Could you help to review?
877fcd6 toda46193Comparecodecovbot commentedJun 13, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## master #1548 +/- ##==========================================+ Coverage 97.33% 98.25% +0.91%========================================== Files 42 58 +16 Lines 2104 2696 +592 ==========================================+ Hits 2048 2649 +601+ Misses 56 47 -9
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| (("1.0.0-reallyweird","PATCH","reallyweird",0,None),"1.0.0-reallyweird1"), | ||
| (("v0.7.1-release","PATCH","release",0,None),"0.7.1-release1"), | ||
| (("v0.0.1-SNAPSHOT","PATCH","SNAPSHOT",0,None),"0.0.1-snapshot1"), |
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.
| (("1.0.0-reallyweird","PATCH","reallyweird",0,None),"1.0.0-reallyweird1"), | |
| (("v0.7.1-release","PATCH","release",0,None),"0.7.1-release1"), | |
| (("v0.0.1-SNAPSHOT","PATCH","SNAPSHOT",0,None),"0.0.1-snapshot1"), | |
| (("1.0.0-reallyweird","PATCH","reallyweird",0,None),"1.0.1-reallyweird1"), | |
| (("v0.7.1-release","PATCH","release",0,None),"0.7.2-release1"), | |
| (("v0.0.1-SNAPSHOT","PATCH","SNAPSHOT",0,None),"0.0.2-snapshot1"), |
I feel they probably should be this instead?
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 need some time to figure out how to adjust the logic.
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.
Isn't the version number already appended at the end of the version string?
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.
@Lee-W a gentle reminder for this discussion
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 honestly don't remember...@woile any idea?
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.
also it might be a good idea to make these named tuple. too hard to read...
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 agree the tests can be more readable. I had#1598 that attempts to make the tests read better.
I can make the NamedTuple change in that PR.
| # See https://github.com/pypa/packaging/blob/14b83e15dbb9caa87c63646ba7808b2b5e460ce6/src/packaging/version.py#L117 | ||
| # TODO: add more test cases for this pattern | ||
| _VERSION_PATTERN=r"""^\s* |
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.
One thing to note is that Python does not use semver. It exists before semver
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.
Should I add any comments here?
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.
not a bad idea. more like a reminder that we can not fully depend on python packaging version
bearomorphism commentedSep 8, 2025
I will update this PR this week when I have bandwidth |
da46193 to3081fffComparebearomorphism commentedSep 9, 2025
Maybe we can adjust the workflow. Several PRs got closed just because the target branch is deleted |
Lee-W commentedSep 9, 2025
Yep, this was a temporary workflow. we usually don't receive this many PR and don't review this fast. Maybe worth rethink it to make a rc to avoid what we encountered yesterday as well |
3081fff to8ee1dbeCompare8ee1dbe tobb48144Compare| (?P<release>[0-9]+(?:\.[0-9]+)*) # release segment | ||
| (?P<pre> # pre-release | ||
| [-_\.]? | ||
| # BEGIN different from packaging.version.py |
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.
Added those comments.
Uh oh!
There was an error while loading.Please reload this page.
Description
Related issue:#950
Checklist