Add tb_version to handle_metadata_tags & Make the version input parser more permissive#7258
Add tb_version to handle_metadata_tags & Make the version input parser more permissive#7258denyshon wants to merge 2 commits intomozilla:mainfrom
Conversation
cd0bc55 to
c5c8d07
Compare
Add tb_version to handle_metadata_tags Make the version input parser more permissive (allow leading/trailing spaces, allow any text after the version number, allow integer version numbers)
|
|
||
| # Remove from the version string anything following the version number. | ||
| # A version number is a sequence of digits and dots, starting and ending with a digit. | ||
| versionMatch = re.match(r'\d(?:[\d.]*\d)?', version) |
There was a problem hiding this comment.
Several comments here:
- Use snake case instead of camel case, so
version_match. - Your regular expression will match multiple periods in a row like (
"148..1.01"). You could avoid that with something liker"\d+(?:\.\d+)*"instead. - In our current code, although
versionremoves suffixes likea1andb2, it keeps suffixes likerc1andrc2. Yourversionstrips therc*suffixes. - As far as I can tell, your
versionwill never match anything in the development releases (firefox_history_development_releasesandthunderbird_history_development_releases), since those only contain versions ending ina*,b*, andrc*. The currentversioncan match versions ending inrc*. - I think with your version, release candidate versions will fall through to the
_has_betahandling, and be labelled asbeta. - So, I think your version regular expression will have to include
rc*suffixes, or maybe just continue usingversion = re.split("[a-b]", version)[0]?
| development_releases = product_details.firefox_history_development_releases or product_details.thunderbird_history_development_releases | ||
| stability_releases = product_details.firefox_history_stability_releases or product_details.thunderbird_history_stability_releases | ||
| major_releases = product_details.firefox_history_major_releases or product_details.thunderbird_history_major_releases |
There was a problem hiding this comment.
These variables will always contain the Firefox releases, because they're always "truthy".
| or version in stability_releases | ||
| or version in major_releases | ||
| ): | ||
| tags.append(product_name + " {}".format(version)) |
There was a problem hiding this comment.
I think it's more readable to use f-strings (f"{product_name} {version}") instead (here and the lines below).
| versionMatch = re.match(r'\d(?:[\d.]*\d)?', version) | ||
| version = versionMatch.group(0) if versionMatch else "" | ||
| # If there are no dots, version is an integer, and we need to add ".0" to find matches among major versions. | ||
| if '.' not in version: |
There was a problem hiding this comment.
If the version is empty, this will make it .0.
|
Hi @escattone, thank you for the review! I've addressed your comments in a follow-up commit (keeping it separate for readability). My only concern is related to "rc" versions. You are right that the current code respects them, and I've updated my regex string to do the same, but... is there any practical reason to do so? From what I can see, every "rc" entry in the development releases has a corresponding normal entry (without "rc") in the stability/major releases, and modern versions don't have "rc" entries at all. If that's no longer needed, we could remove "rc" from the regex and check |
|
Thank you @escattone! Your patch is definitely better, so let's stick to it and close this one |
Resolves #2810, resolves #2811.