977384 - Refactor mach mercurial-setup

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Comment 1

[Ed Morley [:emorley]]

Attachment: Part 1: Abstract prompts/activation for extensions bundled with Mercurial

Comment 2

[Ed Morley [:emorley]]

Attachment: Part 2: Abstract prompts/activation for external extensions

Comment 3

[Ed Morley [:emorley]]

Attachment: Part 3: Misc cleanup

The less daft suggestions from Flake8 + a few other cleanups.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8382668 [details] [diff] [review]
Part 2: Abstract prompts/activation for external extensions

Review of attachment 8382668 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this cleanup!

::: tools/mercurial/hgsetup/wizard.py
@@ +69,5 @@
>  The qimportbz extension
>  (https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hgext/qimportbz/README) makes it possible to
>  import patches from Bugzilla using a friendly bz:// URL handler. e.g.
>  |hg qimport bz://123456|.
> +Would you like to activate qimportbz

Let's throw an extra line break before the prompt line in the 3 of these.

@@ +152,3 @@
>  
>          self.prompt_native_extension(c, 'mq',
>              'Would you like to activate the mq extension to manage patches')

While we're here, let's move mq before extension extensions so we have a nice grouping of built-in then 3rd party.

@@ +254,5 @@
> +            print('Activated %s extension.\n' % name)
> +        if not path:
> +            path = os.path.join(self.vcs_tools_dir, 'hgext', name)
> +            self.update_vcs_tools = True
> +        c.activate_extension(name, path)

I think you want the print('Activated extension') and c.activate_extension() lines to be next to each other like they are for prompt_native_extension().

Comment 5

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Thanks for doing this cleanup!

np :-)

> Let's throw an extra line break before the prompt line in the 3 of these.

Done

> While we're here, let's move mq before extension extensions so we have a
> nice grouping of built-in then 3rd party.

Agreed; done

> @@ +254,5 @@
> > +            print('Activated %s extension.\n' % name)
> > +        if not path:
> > +            path = os.path.join(self.vcs_tools_dir, 'hgext', name)
> > +            self.update_vcs_tools = True
> > +        c.activate_extension(name, path)
> 
> I think you want the print('Activated extension') and c.activate_extension()
> lines to be next to each other like they are for prompt_native_extension().

That would mean we show the "Activated extension" message even if the extension was activated already. The complication with the 'external extension' case other the native one, is that we want to also update the path for extensions that are present in the version-control-tools repo, so can't just bail if the extension was installed.

Or more clearly:

native extension...
already-enabled -> no-op
not enabled, and they said no -> no-op
not enabled, but they said yes -> enable + print activation message

external extension...
already-enabled -> update path to catch those now inside vcs-tools repo + flag vcs-tools repo update
not enabled, and they said no -> no-op
not enabled, but they said yes -> enable + print activation message + flag vcs-tools repo update

Comment 6

[Gregory Szorc [:gps]]

Oh, right. Carry on.

Comment 7

[Ed Morley [:emorley]]

Thank you for the reviews :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aad2d9752a5e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/903893620b9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5fae30b6968f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aad2d9752a5e
https://hg.mozilla.org/mozilla-central/rev/903893620b9f
https://hg.mozilla.org/mozilla-central/rev/5fae30b6968f