Semver support #219
Semver support #219
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 89.81% 90.24% +0.42%
==========================================
Files 41 42 +1
Lines 1974 2009 +35
==========================================
+ Hits 1773 1813 +40
+ Misses 201 196 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ByteOtter
left a comment
ByteOtter
left a comment
There was a problem hiding this comment.
There are some smaller issues with typing and Codecov reports that the test coverage is insufficient (99 lines missing).
The biggest thing is the multiple function declaration though, which I would like to see addressed as this would also be a functional bug though as Python afaik only keeps the last definition it loads.
Personally I would prefer returning an empty string or something like 0.0.0 in case of error instead of None, this makes working with it easier down the line.
Also, maybe for this it would be worth investigating if the packaging.version is worth using here. It's part of the standard library.
| if patch > 0: | ||
| previous_version = f"{major}.{minor}.{patch - 1}" if minor else f"{major}.{patch - 1}" | ||
|
|
||
| output += ( | ||
| f"## Changes in Package Versions Compared to {previous_version}\n" | ||
| ) | ||
| output += compare_apt_repo_versions( | ||
| previous_version, gardenlinux_version | ||
| ) | ||
| elif patch == 0: |
There was a problem hiding this comment.
In these lines patch, major and minor are possibly unbound. Initializing them with a default value would be the easiest way to fix this if this is feasible.
There was a problem hiding this comment.
Probably I'm missing something, but I can't find how they can be unbound. We check explicitly that there are 2 or 3 version components and then assign those major, minor, patch.
There was a problem hiding this comment.
It's about these blocks:
if len(version_components) == 2:
minor = None
patch = int(version_components[1])
if len(version_components) == 3:
minor = int(version_components[1])
patch = int(version_components[2])
Logically speaking you are right and they can't really be unbound in reality. However, pyright remains conservative and cannot infer that the remaining possible lengths are 2 or 3. So it sees the possibility that none of these if statements execute.
The easiest way to fix this would be to simply initialize these value beforehand like this:
major = int(version_components[o])
minor: int | None = None # This one can be missing
patch: int = 0 # Or this one can be int | None depending on the interpretation for versions with only 2 digits.
I just tested it on my machine and it should ™️ fix the issue.
There was a problem hiding this comment.
This issue is not yet been solved.
There was a problem hiding this comment.
I see you initialize the versions before the if statements now which is good. But due to initializing patch with None another issue shows up for anything after if patch > 0, as we never check for if patch is not None.
I'd just go ahead and interpret patch as the second number in a two digit version and treat the value as a number by initializing patch: int = 0 to avoid that. This would mean, if we encounter a two digit version number that we define the first one as major and the second as patch and minor is missing as there are no minor versions when going by two digit versioning.
There was a problem hiding this comment.
I've updated the code to use OOP-style version handling and hopefully make type checker happier. Please recheck.
ByteOtter
left a comment
ByteOtter
left a comment
There was a problem hiding this comment.
I think this is okay for now. So, unless @NotTheEvilOne has something more to add, I think we can merge this.
What this PR does / why we need it:
Adds semver support to release notes generation code.
Which issue(s) this PR fixes:
Fixes gardenlinux/gardenlinux#3645 (comment)