Improve update-versions.py #47
Conversation
pedorich-n
commented
Feb 27, 2024
pedorich-n
commented
- fixes Inconsistent ordering of versions #46
- adds type-hints
| versions_jsonified = OrderedDict( | ||
| (str(version), hashes.__dict__) for version, hashes in versions.items() | ||
| ) |
There was a problem hiding this comment.
This hack is required because json.dump allows only str as keys. So we have to create another OrderedDict (it preserves insert order) with string keys.
There was a problem hiding this comment.
@pedorich-n I think that is fine for the time being.
update-versions.py
Outdated
| } | ||
| return versions | ||
| result: List[semver.Version] = [] | ||
| for release in releases: |
There was a problem hiding this comment.
@pedorich-n I would argue that using high-order functions makes it a little bit easier to reason what this specific section of the code does as it hides some of the implementation details of looping over the elements of the list, filtering, and generating a new list out of the subset of stable releases:
return map(to_version, filter(is_stable, releases))While I understand the proposed approach involves looping through the list of releases only once, I don't think we have a lot of entries right now to worry about performance, so I'm more inclined to favor readability. What are your thoughts?
There was a problem hiding this comment.
As I primarily use FP languages, I agree, but I think in Python FP is not the preferred way of doing things. I've even heard the map, filter, reduce, and other functions were supposed to be removed from Python 3.
But if you don't mind that, I sure can change to map and filter
| import subprocess | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import ( |
There was a problem hiding this comment.
@pedorich-n this is a great improvement! Thank you for adding type-hints throughout the script.
There was a problem hiding this comment.
@pedorich-n I appreciate you taking the effort to work on this issue; the type-hints were an amazing touch! I left one comment, please let me know your thoughts.
|
Hey @pedorich-n! You may want to rebase/merge the |
- fixes stackbuilders#46 - adds type-hints
pedorich-n
force-pushed
the
improve-update-versions
branch
from
3b10c18 to
1218dac
Compare
There was a problem hiding this comment.
@pedorich-n this is a fantastic contribution; apologies for taking so long to merge this one.
