[PATCH 5 of 7 🚿🍦] patchbomb: extract 'getbundlemsgs' closure in its own function

Pierre-Yves David pierre-yves.david at ens-lyon.org
Thu Nov 6 12:54:53 CST 2014



On 11/06/2014 05:58 PM, Martin von Zweigbergk wrote:
> On Thu Nov 06 2014 at 7:47:08 AM Pierre-Yves David
> <pierre-yves.david at ens-lyon.org <mailto:pierre-yves.david at ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david at fb.com
>     <mailto:pierre-yves.david at fb.com>>
>     # Date 1415137703 0
>     #      Tue Nov 04 21:48:23 2014 +0000
>     # Node ID 2d48ca81fd12986424d08a7abd6fda____eaa1d832d7
>     # Parent  7a4add054e1090a3aa88464cf1ca54____751b9d3e36
>     patchbomb: extract 'getbundlemsgs' closure in its own function
>
>     Keep marching toward the promised land of simplification!
>
>     diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
>     --- a/hgext/patchbomb.py
>     +++ b/hgext/patchbomb.py
>     @@ -230,10 +230,35 @@ def _getdescription(repo, defaultbody, s
>               msgfile = repo.opener('last-email.txt', 'wb')
>               msgfile.write(body)
>               msgfile.close()
>           return body
>
>     +def _getbundlemsgs(repo, sender, bundle, **opts):
>     +    """Get the full email sending a given bundle
>     +
>     +    This function return a list of "email" tuple (subject, content,
>     None).
>     +    The list is alway one message long in that case.
>
>
> s/alway/always/

true

> Should the reader be assumed to be know that the "None" is for the
> diffstat? (I'm perfectly fine with a "yes"; it was just not clear to me.)

Lets says yes (the user should be happy to have some docstring ;) )

>           if patches:
>               msgs = getpatchmsgs(patches, opts.get('patchnames'))
>           elif bundle:
>     -        msgs = getbundlemsgs(_getbundle(repo, dest, **opts))
>     +        bundledata = _getbundle(repo, dest, **opts)
>     +        bundleopts = opts.copy()
>     +        bundleopts.pop('bundle', None)  # already processed
>
>
> Sorry, I didn't get this part. What happens without the pop()? Why is it
> needed because the function is extracted?

without the pop we have a conflict between the positional bundle 
argument and the one in **opts.

I should probably have mentionned that in the commit description.

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list