Refactor mach handler management

RESOLVED FIXED in mozilla19

Status

enhancement
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: gps, Assigned: gps)

Tracking

Trunk
mozilla19
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Before: mach was tightly coupled with MozbuildObject
After: mach is more standalone. The only part of mach that is still coupled with mozbuild is the settings integration. Expect another patch for that sometime.

Intimate details about the changes are in the commit message.

All this work is needed to enable more flexibility with how mach calls commands. The old model of a list of arguments to __init__ did not give us any flexibility. The refactor to how handlers are captured will also enable us to implement things like bash shell completion (bug 799680).
Attachment #678064 - Flags: review?(jhammel)
Comment on attachment 678064 [details] [diff] [review]
Refactor mach handler management, v1

Gah. I needed to bit rot myself when I started working on moving settings over. I'll just wait until I have a full patch series before I upload anything...
Attachment #678064 - Flags: review?(jhammel)
Assignee: nobody → gps
Attachment #678064 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #678071 - Flags: review?(jhammel)
We no longer reference mozbuild from within mach \o/

Settings are still disabled in the main mach driver. There will likely be refactorings still. For another bug, I reckon.
Attachment #678072 - Flags: review?(jhammel)
+    __slots__ = (

I have never used __slots__ before in python so am less familiar with their use-case.  Why are they used here?
Comment on attachment 678071 [details] [diff] [review]
Part 1: Refactor mach handler management, v1

+        if not hasattr(args, 'mach_class'):
+            raise Exception('ArgumentParser result missing mach_class.')
+
+        cls = getattr(args, 'mach_class')

I'd probably do:
cls = getattr(args, 'mach_class', None)
if cls is None:
    ...

+Registrar = MachRegistrar()
+

Should this be a singleton?  Perhaps not...

     @Command('throw')
     @CommandArgument('--message', '-m', default='General Error')
     def throw(self, message):
         raise Exception(message)

Out of scope for this bug, but perhaps if no arguments are passed to @Command then it takes the function name?

Looks good outside of me not understanding why slots is used
Attachment #678071 - Flags: review?(jhammel) → review+
Comment on attachment 678072 [details] [diff] [review]
Part 2: Discover settings providers via decorators, v1

lgtm
Attachment #678072 - Flags: review?(jhammel) → review+
(In reply to Jeff Hammel [:jhammel] from comment #4)
> +    __slots__ = (
> 
> I have never used __slots__ before in python so am less familiar with their
> use-case.  Why are they used here?

Typically Python classes store their members in a normal dict. You can add and remove fields in the dict at run-time with no limitations.

__slots__ is syntactic sugar for defining the allowed fields for a class. Instead of a dict you have a fixed set of fields.

There are a few benefits to __slots__. 1) It cuts down on overhead by not allocating a full dictionary. If you have thousands of instances with __slots__ this can add up. 2) It prevents assignment to "unknown" fields. If you attempt to assign to a field that isn't in __slots__ you get a run-time error. 3) It is kinda an API on what fields are available. Instead of looking in the source you can look at __slots__.

I use __slots__ mainly for 2 and 3. It helps catch typos and makes it easy to glance at a class to see what fields are available.
https://hg.mozilla.org/mozilla-central/rev/e0e1220d2c5a
https://hg.mozilla.org/mozilla-central/rev/4f96fdb5e4f9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.