Closed
Bug 808336
Opened 12 years ago
Closed 12 years ago
Refactor mach handler management
Categories
(Firefox Build System :: Mach Core, enhancement)
Firefox Build System
Mach Core
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla19
People
(Reporter: gps, Assigned: gps)
References
Details
Attachments
(2 files, 1 obsolete file)
27.40 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
8.04 KB,
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•12 years ago
|
||
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 | ||
Comment 2•12 years ago
|
||
Assignee: nobody → gps
Attachment #678064 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #678071 -
Flags: review?(jhammel)
Assignee | ||
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
+ __slots__ = (
I have never used __slots__ before in python so am less familiar with their use-case. Why are they used here?
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
Comment on attachment 678072 [details] [diff] [review]
Part 2: Discover settings providers via decorators, v1
lgtm
Attachment #678072 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
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
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•