Issue 6021 - Refactoring build.py

Issue 6021 - Refactoring build.py

[tlucas, 2017-11-15 10:26:28]

Patch Set 1

 * Refactoring build.py to use argparse
 * Purging flake8-ignores for build.py

[Wladimir Palant, 2017-11-17 09:26:11]

https://codereview.adblockplus.org/29609559/diff/29609560/build.py
File build.py (right):

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode49
build.py:49: choices=platforms, required=True)
This is suboptimal for repositories where only one metadata file exists, an
explicit type shouldn't be required then. I suggest that you check existing
metadata files first, same as the old code. You can then build an intersection
between platforms and existing metadata files. If this results in only one entry
the type parameter won't be necessary. Also, if this results in no entries the
command should be omitted altogether.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode79
build.py:79: def run_build(base_dir, build, key_file, release, output_file,
platform,
Nit: Maybe rename this function into build and drop explicit name for the
command? Similarly with the other commands.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode91
build.py:91: import buildtools.packagerEdge as packager
Nit: This is code you merely moved around but consider making the imports here
consistent to create_devenv below.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode155
build.py:155: 'project_key', help='The crowdin project key (required).',
nargs='?'
This is a required parameter, why not nargs=1? This should also make saying
"required" in the description unnecessary.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode167
build.py:167: Set up translation languages for the project on crowdin.net.
Here and for the commands below: it's crowdin.com.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode265
build.py:265: if platform == 'chrome':
Nit: This check can be removed, we have no other platforms.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode302
build.py:302: '-d', '--downloads-repository', dest='download_repository',
Is the naming download_repository rather than downloads_repository intentional?
It's inconsistent with the flag and actual repository name.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode303
build.py:303: default='../downloads',
Please use os.path.join() here. Also, the default path is relative to the
project repository and not cwd. So it's probably best if you leave out the
default here and determine it in the command handler if downloads_repository is
None.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode307
build.py:307: 'version', nargs='?', help='Version number of the release',
This parameter isn't optional.

[tlucas, 2017-11-17 11:36:45]

Patch Set 2:
 * removed explicit subcommand names
 * reintroduced (and refactored) determination of valid platforms based on
present metadata.* files
 * Made single required positional argument actually required and single
 * Addressed some nits

https://codereview.adblockplus.org/29609559/diff/29609560/build.py
File build.py (right):

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode49
build.py:49: choices=platforms, required=True)
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> This is suboptimal for repositories where only one metadata file exists, an
> explicit type shouldn't be required then. I suggest that you check existing
> metadata files first, same as the old code. You can then build an intersection
> between platforms and existing metadata files. If this results in only one
entry
> the type parameter won't be necessary. Also, if this results in no entries the
> command should be omitted altogether.

Done. (See collect_platforms)

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode79
build.py:79: def run_build(base_dir, build, key_file, release, output_file,
platform,
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Nit: Maybe rename this function into build and drop explicit name for the
> command? Similarly with the other commands.

Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode91
build.py:91: import buildtools.packagerEdge as packager
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Nit: This is code you merely moved around but consider making the imports here
> consistent to create_devenv below.

Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode155
build.py:155: 'project_key', help='The crowdin project key (required).',
nargs='?'
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> This is a required parameter, why not nargs=1? This should also make saying
> "required" in the description unnecessary.

nargs=1 would produce a list of one value, leaving us with having to unpack that
list.
This would be fine - however, leaving out any "nargs" parameter has the same
effect of making it required and also only consuming one argument.
Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode167
build.py:167: Set up translation languages for the project on crowdin.net.
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Here and for the commands below: it's http://crowdin.com.

Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode265
build.py:265: if platform == 'chrome':
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Nit: This check can be removed, we have no other platforms.

Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode302
build.py:302: '-d', '--downloads-repository', dest='download_repository',
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Is the naming download_repository rather than downloads_repository
intentional?
> It's inconsistent with the flag and actual repository name.

No, this was not intentional. Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode303
build.py:303: default='../downloads',
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> Please use os.path.join() here. Also, the default path is relative to the
> project repository and not cwd. So it's probably best if you leave out the
> default here and determine it in the command handler if downloads_repository
is
> None.

Done.

https://codereview.adblockplus.org/29609559/diff/29609560/build.py#newcode307
build.py:307: 'version', nargs='?', help='Version number of the release',
On 2017/11/17 09:26:10, Wladimir Palant wrote:
> This parameter isn't optional.

Done.

https://codereview.adblockplus.org/29609559/diff/29611559/build.py
File build.py (right):

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode95
build.py:95: collect_platforms._result = None
I did not see the necessity to introduce a fully fletched singleton-pattern
here. However, if desired i can change this.

[Wladimir Palant, 2017-11-17 11:51:58]

https://codereview.adblockplus.org/29609559/diff/29611559/build.py
File build.py (right):

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode79
build.py:79: 'metadata.* is required.', file=sys.stderr)
This message is confusing now, with only known platforms being considered. Maybe
actually spell out valid files? "Expecting one of metadata.chrome,
metadata.gecko, ..."

Also, a while ago we realized that print (at least in Python 2.x) has trouble
printing Unicode information which is why we decided to use logging module for
stderr output.

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode83
build.py:83: for _, sub_parser in SUB_PARSERS.choices.items():
If you don't need the keys, why iterate over items? .values() should do.

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode89
build.py:89: sub_parser.set_defaults(platform=platforms.pop())
What about len(platforms) == 0?

[tlucas, 2017-11-17 13:32:05]

Patch Set 4
 * Only construct subcommands if the would actually be available
 * Clarified error message when metadata.* is missing
 * Use logging instead of printing to stderr / raising Exceptions

https://codereview.adblockplus.org/29609559/diff/29611559/build.py
File build.py (right):

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode79
build.py:79: 'metadata.* is required.', file=sys.stderr)
On 2017/11/17 11:51:58, Wladimir Palant wrote:
> This message is confusing now, with only known platforms being considered.
Maybe
> actually spell out valid files? "Expecting one of metadata.chrome,
> metadata.gecko, ..."
> 
> Also, a while ago we realized that print (at least in Python 2.x) has trouble
> printing Unicode information which is why we decided to use logging module for
> stderr output.

Done.

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode83
build.py:83: for _, sub_parser in SUB_PARSERS.choices.items():
On 2017/11/17 11:51:58, Wladimir Palant wrote:
> If you don't need the keys, why iterate over items? .values() should do.

This will be obsolete with addressing the below comment.

https://codereview.adblockplus.org/29609559/diff/29611559/build.py#newcode89
build.py:89: sub_parser.set_defaults(platform=platforms.pop())
On 2017/11/17 11:51:58, Wladimir Palant wrote:
> What about len(platforms) == 0?

This was not trivial - but i came up with a solution, which only actually
constructs subcommands if the would be available (see
build_available_subcommands / make_subcommand). Done.

[tlucas, 2017-11-17 14:30:58]

Patch Set 5

* Make platform param required if ambiguous

[Wladimir Palant, 2017-11-18 09:36:38]

LGTM

[tlucas, 2017-11-20 09:18:59]

Patch Set 6:

 * Fixed a regression, '-q' in 'docs' expected a parameter (is now a flag)

[Wladimir Palant, 2017-11-20 11:34:15]

Even more LGTM