Maybe you're right. I don't mind whichever way you name that variable, please do as you see fit.
Cheers, Janos On Mon, Jan 28, 2013 at 2:59 PM, C. Michael Pilato <[email protected]> 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 <[email protected]> > CollabNet <> www.collab.net <> Enterprise Cloud Development > -- Janos Gyerik http://www.janosgyerik.com/ https://twitter.com/janosgyerik/
