Fixing platform_release pep440
Resolves: python-poetry/poetry#7418
- [x] Added tests for changed code.
- [ ] Updated documentation for changed code.
Can add more tests if requested. Any feedback welcome as well, attempted to fix with smallest changes and no side effects.
Examples of conditions this should fix
-
'tegra' in platform_release -
'platform_release <= 5.10.123-tegra
When parsing versions, I added an additional semver-ish regex that will catch patterns that match certain platform_release versions that aren't quite pep440 or semver. Examples are either WSL2 builds, raspi builds or nvidia edge device builds
-
5.15.146.1-microsoft-standard-WSL2 -
6.6.20+rpt-rpi-v8 -
5.10.120-tegra
Interesting approach. I'll try to take a closer look later this week.
For future reference: #552 is a previous attempt to solve the issue by just making platform_release "non-version-like". The comments in this PR might be interesting to understand why it is not that simple.
Because this addresses some issues for folks using edge devices (my team using -tegra nvidia devices), it was important to keep it version-like still. The big takeaways here are:
- Adding a string comparison flag
str_cmpto detect when'substring' in platform_releasefor simple testing - Since OS versions aren't semver like, we choose to cheese it a bit and extract the maj,min,patch components and then relegate the rest of build tag to the
Versionclassespostarg.
Thank you for your review, I will work to fixup my commit flow to match change+unit test and follow up on your comments.
I have forced pushed over my previous commits with 3 commits hopefully tying change+test into each commit. There might be some overlap between tests. Most of the diff is reorganizing commits and included your suggestions.
If you change a method, please take a look if there is a unit test for this method and extend the test.
I added several more tests to the PR by looking at the methods I modified. Please let me know if there are ones I overlooked or additional test cases you would like to see.
Will address comments and fix issues in coming days
I've updated the allows method and tests related to intersection and union. I definitely need a sanity check. I included a logic table to double check myself.
I managed to make some changes to the regex and how we represent the marker internally. Now all markers should be correctly represented.
"tegra" in platform_release is now "tegra" in for the single marker instead of integra
I have hopefully deconflicted correctly the PR that was merged earlier and addressed your comments on the regex.
I was traveling for work, but now I am getting back to this. I will be asking some clarification questions as I work through you latest comments.
I want to get some more test cases in to ensure I have covered edge cases. This topic is indeed finicky
I added a few more tests, fixed and simplified a few things. I think I will take another look next week and - if everything still makes sense to me - finally merge it.
Thanks for your endurance.