CFE-4559: cfbs convert, part 2
#276
Conversation
|
Thanks for submitting a PR! Maybe @craigcomstock can review this? |
larsewi
left a comment
larsewi
left a comment
There was a problem hiding this comment.
Great work 🚀 Some minor comments
|
|
||
| from cfbs.utils import dict_sorted_by_key, file_sha256 | ||
|
|
||
| Version = str |
There was a problem hiding this comment.
You could consider making this a class that inherrits string. You could overwrite the __lt__, __le__, etc. magic functions. Then you could use min() and max() directly on that type.
There was a problem hiding this comment.
Yes, I agree, the intention was to make that change separately, though. Especially since other code uses functions using this type.
| other_versions_files = sorted(other_versions_files) | ||
| files_to_delete = [] | ||
| for ov_file, other_versions in other_versions_files: | ||
| # don't display all versions (which is an arbitrarily-shaped sequence), instead choose the most relevant one: |
There was a problem hiding this comment.
What do you mean by arbitrarily-shaped sequence?
There was a problem hiding this comment.
I meant that since the list of the multiple other versions might be incontiguous, it can't be displayed as a range, for example. So it's best to simply not display all the versions and instead choose one.
… type hints Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
Signed-off-by: jakub-nt <175944085+jakub-nt@users.noreply.github.com>
| # Unfortunately, Pyright can not infer that these can't be `None` when `other_versions` is non-empty. | ||
| assert highest_other_version is not None | ||
| assert lowest_other_version is not None |
There was a problem hiding this comment.
These asserts are repeating, maybe it's more useful if we change highest_version and lowest_version to never return None, i.e. to not accept empty iterables? I checked the other places where highest_version() is used, and it doesn't seem like a bad change.
| def highest_version(versions: Iterable[Version]): | ||
| return max(versions, key=version_as_comparable_list, default=None) |
There was a problem hiding this comment.
Ref the asserts and comments below;
| def highest_version(versions: Iterable[Version]): | |
| return max(versions, key=version_as_comparable_list, default=None) | |
| def highest_version(versions: Iterable[Version]) -> Version: | |
| result = max(versions, key=version_as_comparable_list, default=None) | |
| assert result is not None, "highest_version() does not accept empty lists / iterables" | |
| return result |
And the same for lowest_version() - Then, the return type for these are simpler, and it's incorrect to call them with an empty list.
4 participants
No description provided.