On Jan 28, 2013 3:59 AM, "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
Seriously? 14 chars instead of 7. To satisfy some naming thought which
wouldn't be abundantly clear from the doc?
repodir seems totally fine.
Received on 2013-01-28 19:07:18 CET