Closed Bug 808336 Opened 12 years ago Closed 12 years ago

Refactor mach handler management

Categories

(Firefox Build System :: Mach Core, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla19

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(2 files, 1 obsolete file)

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: 12 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.