Issue 29348818: issue 4087 - Use Mozilla's product-details data to determine current product versions (Closed)
|
Created:
July 28, 2016, 7:42 p.m. by Jon Sonesen Modified:
Sept. 7, 2016, 12:16 p.m. Visibility:
Public. |
DescriptionReplaces update server calls with simple current version json requests
Patch Set 1 : Replace update server calls with json request #
Total comments: 2
Patch Set 2 : Fix erroneous diff upload #
Total comments: 10
Patch Set 3 : Add suffix removal regex and reorder seamonkey dev versions #Patch Set 4 : Fix regex, add error handling #
Total comments: 8
Patch Set 5 : addresses concerns for maintainability, and redundancies #
Total comments: 2
Patch Set 6 : fixed problems with commas and long line...sorry for the mistake #MessagesTotal messages: 19
(BTW, please always sent mail, when uploading a new patch for review. Otherwise reviewers won't be notified) https://codereview.adblockplus.org/29348818/diff/29348831/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29348831/globals/get_browser... globals/get_browser_versions.py:37: doc = json.loads(response.read()) Any reason you cannot parse the response lazily? doc = json.load(response) https://codereview.adblockplus.org/29348818/diff/29348831/globals/get_browser... globals/get_browser_versions.py:81: def get_seamonkey_version(): This code duplication isn't great. How about, instead having one function that takes the product name (as given in the filename) as argument?
Here is my solution to address code duplication and lazy load of response
Something seems to be wrong with patch set 2. The patch itself shows a few seemingly unrelated changes: https://codereview.adblockplus.org/29348818/patch/29349644/29349645 Moreover, if I view the delta to the previous patch set, it seems like almost everything changed: https://codereview.adblockplus.org/29348818/diff2/29348831:29349644/globals/g... Is that what I'm supposed to see? If not, can you please remove patch set 2 and upload it again, making sure that the patch is complete?
Here is my solution to address code duplication and lazy load of response
Here is my solution to address code duplication and lazy load of response
Here is my solution to address code duplication and lazy load of response
https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:36: 'https://product-details.mozilla.org/firefox_versions.json', Gotta love it - the URL changed into https://product-details.mozilla.org/1.0/firefox_versions.json without a redirect (same with the Thunderbird versions) ;) https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:57: versions['FIREFOX_NIGHTLY'], We need to remove the "a1" and similar suffixes from the version numbers - we only want the actual milestone. https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:70: firefox_versions['FIREFOX_NIGHTLY'], There is LATEST_THUNDERBIRD_ALPHA_VERSION in the data, we should use that rather than FIREFOX_AURORA even though it seems to be outdated right now. https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:81: # https://bugzilla.mozilla.org/show_bug.cgi?id=1086553 This comment no longer makes sense, it doesn't matter whether their update server is working. https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:86: seamonkey_versions['LATEST_SEAMONKEY_DEVEL_VERSION'], This should be the last entry in the list since it seems to designate SeaMonkey nightlies (yes, they aren't being consistent with Firefox).
https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:36: 'https://product-details.mozilla.org/firefox_versions.json', On 2016/08/12 11:40:55, Wladimir Palant wrote: > Gotta love it - the URL changed into > https://product-details.mozilla.org/1.0/firefox_versions.json without a redirect > (same with the Thunderbird versions) ;) will update :D https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:57: versions['FIREFOX_NIGHTLY'], On 2016/08/12 11:40:55, Wladimir Palant wrote: > We need to remove the "a1" and similar suffixes from the version numbers - we > only want the actual milestone. Just to clarify; do you mean that I should remove any alpha numeric leading suffixes? Or rather that anything past the second decimal point should be removed? https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:70: firefox_versions['FIREFOX_NIGHTLY'], On 2016/08/12 11:40:55, Wladimir Palant wrote: > There is LATEST_THUNDERBIRD_ALPHA_VERSION in the data, we should use that rather > than FIREFOX_AURORA even though it seems to be outdated right now. Will use that instead.
https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:57: versions['FIREFOX_NIGHTLY'], On 2016/08/16 02:27:54, Jon Sonesen wrote: > On 2016/08/12 11:40:55, Wladimir Palant wrote: > > We need to remove the "a1" and similar suffixes from the version numbers - we > > only want the actual milestone. > > Just to clarify; do you mean that I should remove any alpha numeric leading > suffixes? Or rather that anything past the second decimal point should be > removed? The code you are replacing here was using r'^(\d+)(?:\.\d+)?' regular expression to extract the version number. It's probably best if you do the same.
https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349782/globals/get_browser... globals/get_browser_versions.py:57: versions['FIREFOX_NIGHTLY'], On 2016/08/16 10:40:17, Wladimir Palant wrote: > On 2016/08/16 02:27:54, Jon Sonesen wrote: > > On 2016/08/12 11:40:55, Wladimir Palant wrote: > > > We need to remove the "a1" and similar suffixes from the version numbers - > we > > > only want the actual milestone. > > > > Just to clarify; do you mean that I should remove any alpha numeric leading > > suffixes? Or rather that anything past the second decimal point should be > > removed? > > The code you are replacing here was using r'^(\d+)(?:\.\d+)?' regular expression > to extract the version number. It's probably best if you do the same. Thank you, also I added a log statement in case they change the url with no redirect, additionally added a value check for the regex in case they add keys that have null strings.
In general, I'm happy with the logic. There are a couple code quality issues that I'm a bit confused about: they are not a big deal and perhaps it's too much to demand from a macro inside of a website. But then, this macro is rather big, so perhaps we should worry about maintainability, etc. Anyway, see below: https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:34: urls = { This dict seems somewhat redundant here. Wouldn't it be better to move these urls into constants at the top level of the file and then use them directly in `get_xxx_versions` functions? Kind of like this: def get_json_versions(url): ... FIREFOX_URL = '....' def get_firefox_versions(): versions = get_json_versions(FIREFOX_URL) Then `get_json_versions` becomes more generic and the hardcoded URLs end up in constants instead of the dict. I know that in other functions the URLs are hardcoded directly into the calls, but here we'd be using FIREFOX_URL twice and, well, having the URLs in the constants seems cleaner anyway. https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:52: doc[key] = re.search(r'^(\d+)(?:\.\d+)?', value).group(0) Isn't this a bit fragile? What if the regex doesn't match? https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:61: versions['LATEST_FIREFOX_DEVEL_VERSION'], It would be nice to give more friendly error messages in case the key is not there, but all other browser version getters don't really take care of this. Probably makes sense to follow suit here for consistency. https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:75: versions['LATEST_THUNDERBIRD_ALPHA'], I think this should be LATEST_THUNDERBIRD_ALPHA_VERSION
I agree with you about making the code more maintainable. With the regex thing I can add an error message if we want or have it fail if there aren't minor versions to strip. Just let me know :D https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:34: urls = { On 2016/08/22 17:51:55, Vasily Kuznetsov wrote: > This dict seems somewhat redundant here. Wouldn't it be better to move these > urls into constants at the top level of the file and then use them directly in > `get_xxx_versions` functions? Kind of like this: > > def get_json_versions(url): > ... > > FIREFOX_URL = '....' > def get_firefox_versions(): > versions = get_json_versions(FIREFOX_URL) > > Then `get_json_versions` becomes more generic and the hardcoded URLs end up in > constants instead of the dict. I know that in other functions the URLs are > hardcoded directly into the calls, but here we'd be using FIREFOX_URL twice and, > well, having the URLs in the constants seems cleaner anyway. So rather than a constant dict just have the function call using the constant url as the product? I am happy to do this :D https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:52: doc[key] = re.search(r'^(\d+)(?:\.\d+)?', value).group(0) On 2016/08/22 17:51:55, Vasily Kuznetsov wrote: > Isn't this a bit fragile? What if the regex doesn't match? I agree that it is fragile...but also I thought we wanted this to fail 'loudly' which this would be doing. Also, this is how the code was originally, also, https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:61: versions['LATEST_FIREFOX_DEVEL_VERSION'], On 2016/08/22 17:51:55, Vasily Kuznetsov wrote: > It would be nice to give more friendly error messages in case the key is not > there, but all other browser version getters don't really take care of this. > Probably makes sense to follow suit here for consistency. I thought we decided not to so the build 'fails loudly' basically meaning we need to go investigate why there is not the expected key in case they change something. https://codereview.adblockplus.org/29348818/diff/29349883/globals/get_browser... globals/get_browser_versions.py:75: versions['LATEST_THUNDERBIRD_ALPHA'], On 2016/08/22 17:51:55, Vasily Kuznetsov wrote: > I think this should be LATEST_THUNDERBIRD_ALPHA_VERSION you're right....dang haha
addresses concerns for maintainability, and redundancies
https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... File globals/get_browser_versions.py (right): https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... globals/get_browser_versions.py:14: FIREFOX_URL = 'https://product-details.mozilla.org/1.0/firefox_versions.json', The comma in the end is not only redundant, it actually breaks the code. Please check if the code works before submitting it for review -- the reviewers might not always do it and unfortunately we have no CI (yet!) to catch these things. https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... globals/get_browser_versions.py:15: THUNDERBIRD_URL = 'https://product-details.mozilla.org/1.0/thunderbird_versions.json', This line is too long according to our guidelines (same as PEP8, max 79 chars). The fix would be to do something like: THUNDERBIRD_URL = ('......' '.......') Since this is pretty ugly, you might choose to hack your way out by doing something like this: MOZILLA_BASE = 'https://product-details.mozilla.org/1.0/' FIREFOX_URL = MOZILLA_BASE + 'firefox_versions.json' THUNDERBIRD_URL = MOZILLA_BASE + 'thunderbird_versions.json'
On 2016/08/29 17:27:52, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... > File globals/get_browser_versions.py (right): > > https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... > globals/get_browser_versions.py:14: FIREFOX_URL = > 'https://product-details.mozilla.org/1.0/firefox_versions.json', > The comma in the end is not only redundant, it actually breaks the code. > > Please check if the code works before submitting it for review -- the reviewers > might not always do it and unfortunately we have no CI (yet!) to catch these > things. > > https://codereview.adblockplus.org/29348818/diff/29350293/globals/get_browser... > globals/get_browser_versions.py:15: THUNDERBIRD_URL = > 'https://product-details.mozilla.org/1.0/thunderbird_versions.json', > This line is too long according to our guidelines (same as PEP8, max 79 chars). > > The fix would be to do something like: > > THUNDERBIRD_URL = ('......' > '.......') > > Since this is pretty ugly, you might choose to hack your way out by doing > something like this: > > MOZILLA_BASE = 'https://product-details.mozilla.org/1.0/' > FIREFOX_URL = MOZILLA_BASE + 'firefox_versions.json' > THUNDERBIRD_URL = MOZILLA_BASE + 'thunderbird_versions.json' Sorry for that....I am using syntax checker I just did a dumb thing :(
fixed problems with commas and long line...sorry for the mistake
On 2016/08/30 09:14:47, Jon Sonesen wrote: > fixed problems with commas and long line...sorry for the mistake LGTM
LGTM 
|
