Raise NotFoundError if no daily build can be found for latest build date #672
Conversation
There was a problem hiding this comment.
Thank you for this PR @piri-p! The changes make sense and it also works fine as well. I've just a small nit (see inline).
Also I would appreciate if you could add an automated test here as well. I think that we even do not have these for other applications like Firefox and Thunderbird. So it would be good to also include tests for these as well. Thanks.
|
Thanks for your feedback @whimboo ! I have updated the code and add test cases. |
mozdownload/scraper.py
Outdated
| @@ -451,6 +451,8 @@ def get_latest_build_date(self): | |||
| months.entries[-1] + '/') | |||
| parser = self._create_directory_parser(url) | |||
| parser.entries = parser.filter(r'.*%s' % self.platform_regex) | |||
| if not parser.entries: | |||
| raise errors.NotSupportedError('No builds have been found') | |||
There was a problem hiding this comment.
I just noticed that this should be a NotFoundError. Looks like it slipped through in my last review. Can you please update both the code here and the test?
Beside that we probably should indeed have a NotSupportedError thrown when an unsupported platform has been specified. But this check should be done as early as possible when we know the application and which kind of platform is selected. I would suggest that you create a separate PR for that and we leave open the issue even with this PR merged.
|
Updated to NotFoundError :) @whimboo |
whimboo
changed the title
|
Apologies, just noticed that I forgot to put url, which is required for the NotFoundError (but not for the previous NotSupportedError) - I just added it now and local |
whimboo
changed the title
| import mozdownload.errors as errors | ||
|
|
||
| @pytest.mark.parametrize('args', [ | ||
| ({'application': 'fenix', 'platform': 'mac'}), |
There was a problem hiding this comment.
Please note that with the follow-up PR for NotSupportedError this will change given that mac is a not supported platform. But I'm fine with landing this for now, given that it only affects Fenix builds.
|
Thank you @piri-p for the patch! |
Partly fixes #664. @whimboo