On Thu, May 08, 2014 at 11:36:29PM -0500, Felipe Contreras wrote:
> William Giokas wrote:
> > E401: Multi-line imports seems like something that would just be
> > changing one line
>
> Yes, and make the code very annoying.
It's 1 extra line in git-remote-hg, and 4 lines in git-remote-bzr. I'll
even send in patches for the current version you have in your tree if
you'd like. It would make testing removal of an import easier, and the
adding of imports would be consistent. They can still be grouped, but
having them on a single line kind of limits the ease of manipulation for
testing and possible packager patching if needed.
> > E302: Blank lines don't seem to be that hard to do either. That can even
> > be automated quite reliably. It shouldn't detract from the readability,
> > juts makes the file a bit longer.
>
> The problem is not that it's hard to do, the problem is that it makes
> the code uglier.
I would disagree, but this is one of the less important things.
> > E20{1,2,3}: Extra whitespace is something that just makes things more
> > consistent and readable.
>
> I don't see how this:
>
> {'100755': 'x', '120000': 'l'}
>
> Is more readable than this:
>
> { '100755': 'x', '120000': 'l' }
>
> No strong opinion on this one though.
It's not so much that it's wrong or less readable, but there is
inconsistency on this one and I'd err pep8. Again, will send a patch to
your tree for you to review, though it looks like you mostly fixed this
in [1].
>
> > E12{6,8}: continuation line indenting is another thing that helps
> > consistency.
>
> I don't see how.
>
> > > max-line-length = 160
> >
> > The standard states that this should, at most, be increased to a value
> > between 80 and 100.
>
> And why's that?
>
> This has been discussed many times in the LKML, and the end result is
> that we don't live in the 60's, our terminals are not constrained to 60
> characters. Going beyond 100 is fine.
Fair enough. At the same time, it'd only change 14 lines in the current
git.git tree and would probably increase the readability of some of the
sections. I noticed that some of the changes in the referenced patch
actually fixed this on a few lines as well.
[1]: https://github.com/felipec/git/commit/12374c0e09a84945a202bb5ba7981a223d233d0b
Thanks,
--
William Giokas | KaiSforza | http://kaictl.net/
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF
Attachment:
pgpCpVO88_NoA.pgp
Description: PGP signature