[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] minor change to mailer.py for subject formatting

From: Janos Gyerik <janos.gyerik_at_gmail.com>
Date: Mon, 28 Jan 2013 15:21:01 +0100

Maybe you're right. I don't mind whichever way you name that variable,
please do as you see fit.


On Mon, Jan 28, 2013 at 2:59 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> On 01/27/2013 10:02 AM, Janos Gyerik wrote:
>> If it helps, here's a last try:
>> - wrapped the 3 long lines to 80 columns
>> - PEP8 fixes (only on these 3 lines), such as removed space after "{",
>> before ":", before "}"
>> The only PEP8 violation that remain on these lines is "continuation
>> line under-indented for visual indent"
>> I don't know how to format best, please do as you see fit.
> I can't speak for the PEPs, but prefer to see function parameters and
> dictionary keys left-aligned:
> cfg = Config(config_fname, repos,
> {'author': repos.author,
> 'repodir': os.path.basename(repos.repos_dir),
> })
> But that's just me. I think the formatting you used was fine.
> My only remaining minor nit is the variable name itself. First, other
> variable names in this script use underscores to separate words: for_repos,
> label_from, etc. Secondly, it's really only the basename of the directory
> that's being substituted in, and I think the variable name should carry that
> information. (I had to read your docs to make sure I wouldn't wind up with
> "[svn-/path/to/my/repository commit]".) So, I'd suggest using
> "repos_basename" rather than "repodir". Again, it's a minor thing -- and
> one that the patch applier can probably make on your behalf -- but if the
> name caused me some initial confusion, it'll probably do the same to someone
> else, too.
> --
> C. Michael Pilato <cmpilato_at_collab.net>
> CollabNet <> www.collab.net <> Enterprise Cloud Development

Janos Gyerik
Received on 2013-01-28 15:21:34 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.