1399819 - [compare-locales] simplify command line handler

Status: RESOLVED FIXED

(Localization Infrastructure and Tools :: compare-locales, enhancement)

Description

[Axel Hecht [:Pike]]

Right now, the code in commands.py is a bit entangled.

I'd like to re-use that code in order to make the mach command in m-c compatible with the command in compare-locales proper.

So let's clean that up a bit.

Comment 1

[Axel Hecht [:Pike]]

Attachment: bug 1399819, refactor commands, stop using base class,

Review commit: https://reviewboard.mozilla.org/r/179816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179816/

Comment 2

[Axel Hecht [:Pike]]

Attachment: bug 1399819, separate command handler from argument parsing,

Review commit: https://reviewboard.mozilla.org/r/179818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179818/

Comment 3

[Axel Hecht [:Pike]]

Attachment: bug 1399819, pass arguments explicitly to handle(),

mach ends up calling the command with **vars(args), so we get
all parameters as named params in mach. Do the same in
compare-locales so that we can re-use the code.

Review commit: https://reviewboard.mozilla.org/r/179820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179820/

Comment 4

[Staś Małolepszy :stas]

Comment on attachment 8908138 [details]
bug 1399819, refactor commands, stop using base class,

https://reviewboard.mozilla.org/r/179816/#review185444

Comment 5

[Staś Małolepszy :stas]

Comment on attachment 8908139 [details]
bug 1399819, separate command handler from argument parsing,

https://reviewboard.mozilla.org/r/179818/#review185446

Comment 6

[Staś Małolepszy :stas]

Comment on attachment 8908140 [details]
bug 1399819, pass arguments explicitly to handle(),

https://reviewboard.mozilla.org/r/179820/#review185448

::: compare_locales/commands.py:91
(Diff revision 1)
>          logging.getLogger().setLevel(logging.WARNING -
>                                       (args.v - args.q) * 10)
> -        return self.handle(args)
> -
> -    def handle(self, args):
> +        kwargs = vars(args)
> +        # strip handeld arguments
> +        for arg in ('v', 'q'):
> +            kwargs.pop(arg)

This is a tiny nit, but perhaps simply two `kwargs.pop` calls would be more readable here.

::: compare_locales/commands.py:105
(Diff revision 1)
>          # locales
> -        all_args = args.config + [args.l10n_base_dir] + args.locales
> -        del args.config[:]
> -        del args.locales[:]
> +        all_args = config_paths + [l10n_base_dir] + locales
> +        config_paths = []
> +        locales = []
> +        if defines is None:
> +            defines = []

Nit: since you're not appending to `defines` anywhere here, you could specify `[]` as the default value in the signature.  I'm not sure how Pythonic that would be though.

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 8908138 [details]
bug 1399819, refactor commands, stop using base class,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179816/diff/1-2/

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 8908139 [details]
bug 1399819, separate command handler from argument parsing,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179818/diff/1-2/

Comment 9

[Axel Hecht [:Pike]]

Comment on attachment 8908140 [details]
bug 1399819, pass arguments explicitly to handle(),

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179820/diff/1-2/

Comment 10

[Axel Hecht [:Pike]]

Thanks.

I fixed the pop, but I'm strict on myself and mutable default params. Even if it wouldn't matter in this case.

https://hg.mozilla.org/l10n/compare-locales/pushloghtml?changeset=336be2d55e99