John Arbash Meinel has voted +1 (conditional).
Status is now: Conditionally approved
Comment:
I'm guessing we want to change the format from 'experimental-1' once it
is merged into bzr.dev.
You don't have a doc string on the MergeDirective class, or on any of
its functions.
It seems like the patch_type handler should be using a dictionary rather
than if/elif statements. But at the very least, you need an 'else:' in
there to handle unknown patch formats.
'import smtplib' should really be done at the top of
test_merge_directive, rather than in the middle of a function.
It would be easier to follow if you broke the test up into multiple
tests.
It seems that the default diff you generate is using the 'a/' and 'b/'
prefixes. I believe this is simply because the default for the internal
diff function is to use prefixes.
You don't have to separately decode the 'sign' character. You can just
do:
'%+04d' % (offset,)
That will put +3000 if the number is positive, and -3000 as appropriate.
(I'm happy to see those functions moved into a separate helper, though I
wonder if we need to import them into the old location for
compatibility)
We should probably have a specific test on the serialized output (the
text form), since this is a transmitted text, we need to make sure it
doesn't change between releases. It seems like you have a test at the
EMAIL layer, though, so maybe that is enough.
The indenting on cmd_merge_directive.takes_options is a little hard to
read. I would prefer if the nested scope had extra indentation.
We should promote "public_branch" to being an api call, just like we do
for 'get_submit_branch' and others. Since we have used it enough times
that we know it is something we want to have in core. I won't block this
on doing it, but we should keep it in mind.
One problem with gpg signing a patch, is that it makes the '-' lines a
little ugly. Specifically:
- -def format_highres_date(t, offset=0):
- - """Format a date, such that it includes higher precision in the
- - seconds field.
This doesn't have a whole lot to do with your patch, but it is probably
something that we want to teach BundleBuggy about.
Is there a reason you used this indentation:
+ @staticmethod
+ def from_kwargs(name_, help=None, title=None, value_switches=False,
+ enum_switch=True, **kwargs):
+ """Convenience method to generate string-map registry options
Instead of:
+ @staticmethod
+ def from_kwargs(name_, help=None, title=None, value_switches=False,
+ enum_switch=True, **kwargs):
+ """Convenience method to generate string-map registry options
I generally prefer the latter, but if there is a reason to use the
former...
You need docstrings for the new functions in rio.py
Also calling them "to_patch_lines" and "read_patch_stanza" doesn't quite
fit. These really aren't "patch lines". Maybe "to_width_limited" or
something like that.
For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C45EDAB88.6010305%40utoronto.ca%3E