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

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