Closed Bug 977384 Opened 6 years ago Closed 6 years ago

Refactor mach mercurial-setup

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(3 files)

No description provided.
Attachment #8382634 - Flags: review?(gps) → review+
The less daft suggestions from Flake8 + a few other cleanups.
Attachment #8382671 - Flags: review?(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().
Attachment #8382668 - Flags: review?(gps) → review+
Attachment #8382671 - Flags: review?(gps) → review+
(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
Oh, right. Carry on.
Blocks: 977961
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.