On 01/28/2013 12:29 PM, Daniel Shahaf wrote: > Ben Reser wrote on Mon, Jan 28, 2013 at 09:25:30 -0800: >> On Mon, Jan 28, 2013 at 5:59 AM, C. Michael Pilato <[email protected]> >> wrote: >>> 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. >> >> I almost said something about the name too when I reviewed it. I let >> it go since you could easily say repodir means the name of the >> directory that the repo is in. I'd have gone with repos_name >> personally. >> >> But at this point I feel like we're painting the bikeshed. > > I'll commit with cmpilato's tweaks (indentation and varname) as I agree > with them both. >
Cool. Thanks, Daniel. And thanks, Janos, for the contribution and patient survival of our iterative patch review process. :-) -- C. Michael Pilato <[email protected]> CollabNet <> www.collab.net <> Enterprise Cloud Development
signature.asc
Description: OpenPGP digital signature
