Re: Conforming to pep8
William Giokas wrote:
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.
Ah, it refers to the '^import *' not '^from x import'. That's fine then.
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', '12': 'l'}
Is more readable than this:
{ '100755': 'x', '12': '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].
I don't see inconsistency within the script. All the hashes are in the
form of { content }.
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.
If the result is not so horrible I would consider this, otherwise I'll
just ignore the warning.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
On Fri, May 09, 2014 at 02:18:54AM -0500, Felipe Contreras wrote: William Giokas wrote: 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. Ah, it refers to the '^import *' not '^from x import'. That's fine then. Yeah. In fact, for the mercurial stuff the `from mercurial import changegroup` line should be on the same line as the other `from mercurial import ...` line. Thanks, -- William Giokas | KaiSforza | http://kaictl.net/ GnuPG Key: 0x73CD09CF Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF pgpqk7VXuLc5p.pgp Description: PGP signature
Re: Conforming to pep8
William Giokas wrote: On Fri, May 09, 2014 at 02:18:54AM -0500, Felipe Contreras wrote: William Giokas wrote: 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. Ah, it refers to the '^import *' not '^from x import'. That's fine then. Yeah. In fact, for the mercurial stuff the `from mercurial import changegroup` line should be on the same line as the other `from mercurial import ...` line. I think the line is too big, it should be rearranged. But every time I add something to that list I say: I'll clean it up later =/ -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
On Fri, May 09, 2014 at 02:35:34AM -0500, Felipe Contreras wrote: William Giokas wrote: Yeah. In fact, for the mercurial stuff the `from mercurial import changegroup` line should be on the same line as the other `from mercurial import ...` line. I think the line is too big, it should be rearranged. But every time I add something to that list I say: I'll clean it up later =/ Maybe a time to use something like:: from mercurial import foo \ bar \ baz \ ... Would make that import into quite a few lines, but would help organize things and let you easily organize things in the future. Thanks, -- William Giokas | KaiSforza | http://kaictl.net/ GnuPG Key: 0x73CD09CF Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF pgpvAwdo_OmXt.pgp Description: PGP signature
Re: Conforming to pep8
On Thu, May 08, 2014 at 08:54:29PM -0500, William Giokas wrote: So I have been looking into the python code in the git tree recently (contrib and core tree) and noticed that almost none of the files fully conform to pep8. Now I'm not just saying this because I like the code to be clean, readable and easily parsed by humans, but also because this is laid out in the coding style document that is distributed with the git source:: For Python scripts: - We follow PEP-8 (http://www.python.org/dev/peps/pep-0008/). It's even the first thing that you see when you go looking for 'python' in the coding style document. I just ran every file in the tree that either ended in '.py' or had a python #!, and was greeted with a whole bunch of output:: [snip] Which is a whole bunch of errors and warnings thrown by pep8. Is pep8 just getting put by the wayside? I would much rather have these scripts conform to that and have an actual coding style rather than just be a hodge-podge of different styles. The note about PEP-8 was only added to the CodingStyle document fairly recently, so it's not that it is getting put by the wayside but rather that no one has tidied up what was there before this decision was made, which gets caught under the catch all rule at the top of CodingGuidelines: - Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style. Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up. Cf. http://article.gmane.org/gmane.linux.kernel/943020 Of course, pushing an apply PEP-8 to the entire file patch as the first part of a series that does something else may be worse than fixing up the style at a time when the script is not otherwise subject to change. I suspect the part of CodingGuidelines I quoted above applies more to local style issues than change indentation from tabs to spaces across an entire file. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
On Fri, May 09, 2014 at 02:44:02AM -0500, William Giokas wrote: Maybe a time to use something like:: from mercurial import foo \ bar \ baz \ ... Would make that import into quite a few lines, but would help organize things and let you easily organize things in the future. From PEP 8 [1]: The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation. So I prefer something like: from mercurial import ( bar, baz, foo, ) The indentation for the closing parenthesis is optional [2]. You can of course do things like: from mercurial import ( bar, baz, foo, ) but I prefer the complete specification of “single, alphebetized entry per line”. I'm happy to send patches if that style is ok. Cheers, Trevor [1]: http://legacy.python.org/dev/peps/pep-0008/#maximum-line-length [2]: http://legacy.python.org/dev/peps/pep-0008/#indentation -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: Conforming to pep8
W. Trevor King wrote: The indentation for the closing parenthesis is optional [2]. You can of course do things like: from mercurial import ( bar, baz, foo, ) I prefer: from mercurial foo, bar, ... from mercurial baz, ... -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
(cc-ing Pete Wyckoff who maintains git-p4 and Michael Haggerty
who maintains git-multimail)
Hi,
William Giokas wrote:
- We follow PEP-8 (http://www.python.org/dev/peps/pep-0008/).
It's even the first thing that you see when you go looking for 'python'
in the coding style document. I just ran every file in the tree that
either ended in '.py' or had a python #!, and was greeted with a whole
bunch of output::
./git-p4.py
./contrib/svn-fe/svnrdump_sim.py
./contrib/remote-helpers/git-remote-bzr
./contrib/hooks/multimail/post-receive
./contrib/hooks/multimail/migrate-mailhook-config
./contrib/hooks/multimail/git_multimail.py
./contrib/hooks/multimail/README
./contrib/hg-to-git/hg-to-git.py
./contrib/gitview/gitview
./contrib/fast-import/import-zips.py
Thanks for running this check. Passing on the result to the
maintainers of some of those scripts in case they have thoughts.
As someone involved in contrib/svn-fe/, I would be happy to take a
patch making svnrdump_sim.py follow PEP-8, if you have time to write
one.
Thanks,
Jonathan
List of warnings kept below for reference.
20 E101 indentation contains mixed spaces and tabs
90 E111 indentation is not a multiple of four
9 E112 expected an indented block
47 E113 unexpected indentation
1 E121 continuation line under-indented for hanging indent
3 E122 continuation line missing indentation or outdented
3 E124 closing bracket does not match visual indentation
12 E125 continuation line with same indent as next logical line
9 E126 continuation line over-indented for hanging indent
4 E127 continuation line over-indented for visual indent
63 E128 continuation line under-indented for visual indent
4 E129 visually indented line with same indent as next logical line
3 E131 continuation line unaligned for hanging indent
37 E201 whitespace after '['
30 E202 whitespace before ']'
30 E203 whitespace before ':'
37 E211 whitespace before '('
10 E221 multiple spaces before operator
14 E222 multiple spaces after operator
8 E223 tab before operator
1 E224 tab after operator
35 E225 missing whitespace around operator
6 E228 missing whitespace around modulo operator
23 E231 missing whitespace after ','
10 E251 unexpected spaces around keyword / parameter equals
1 E261 at least two spaces before inline comment
1 E262 inline comment should start with '# '
37 E265 block comment should start with '# '
1 E301 expected 1 blank line, found 0
117 E302 expected 2 blank lines, found 1
19 E303 too many blank lines (2)
4 E401 multiple imports on one line
220 E501 line too long (83 79 characters)
5 E502 the backslash is redundant between brackets
33 E701 multiple statements on one line (colon)
11 E702 multiple statements on one line (semicolon)
34 E703 statement ends with a semicolon
9 E711 comparison to None should be 'if cond is None:'
2 E713 test for membership should be 'not in'
1022W191 indentation contains tabs
40 W601 .has_key() is deprecated, use 'in'
1 W602 deprecated form of raising exception
1 W603 '' is deprecated, use '!='
1 W604 backticks are deprecated, use 'repr()'
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Conforming to pep8
William Giokas wrote:
Which is a whole bunch of errors and warnings thrown by pep8. Is pep8
just getting put by the wayside? I would much rather have these
scripts conform to that and have an actual coding style rather than
just be a hodge-podge of different styles.
Personally I try to follow pep8 in git-remote-{hg,bzr}, but only to some
extent.
I do this:
[pep8]
ignore = E401,E302,E201,E202,E203,E126,E128
max-line-length = 160
That said there's a couple of issues present that I didn't notice.
Thanks for checking.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
On Thu, May 08, 2014 at 09:10:25PM -0500, Felipe Contreras wrote:
William Giokas wrote:
Which is a whole bunch of errors and warnings thrown by pep8. Is pep8
just getting put by the wayside? I would much rather have these
scripts conform to that and have an actual coding style rather than
just be a hodge-podge of different styles.
Personally I try to follow pep8 in git-remote-{hg,bzr}, but only to some
extent.
I do this:
[pep8]
ignore = E401,E302,E201,E202,E203,E126,E128
(So I haven't looked at git-remote-bzr, but I can comment on
git-remote-hg)
Is there a reason for these?
E401: Multi-line imports seems like something that would just be
changing one line
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.
E20{1,2,3}: Extra whitespace is something that just makes things more
consistent and readable.
E12{6,8}: continuation line indenting is another thing that helps
consistency.
max-line-length = 160
The standard states that this should, at most, be increased to a value
between 80 and 100.
Note that I'm not trying to yell, but these are just things that are set
forth in pep8 but don't seem to be set at all in git-remote-hg. I really
think that git's python 'bits' should be able to pass a default pep8
without issue.
That said there's a couple of issues present that I didn't notice.
Thanks for checking.
Hope to see some improvements! git-remote-hg is really quite useful for
me, and I hope it can be as good as possible!
Thanks,
--
William Giokas | KaiSforza | http://kaictl.net/
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF
pgpI8HNeFYEVx.pgp
Description: PGP signature
Re: Conforming to pep8
William Giokas wrote:
On Thu, May 08, 2014 at 09:10:25PM -0500, Felipe Contreras wrote:
William Giokas wrote:
Which is a whole bunch of errors and warnings thrown by pep8. Is pep8
just getting put by the wayside? I would much rather have these
scripts conform to that and have an actual coding style rather than
just be a hodge-podge of different styles.
Personally I try to follow pep8 in git-remote-{hg,bzr}, but only to some
extent.
I do this:
[pep8]
ignore = E401,E302,E201,E202,E203,E126,E128
(So I haven't looked at git-remote-bzr, but I can comment on
git-remote-hg)
Is there a reason for these?
E401: Multi-line imports seems like something that would just be
changing one line
Yes, and make the code very annoying.
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.
E20{1,2,3}: Extra whitespace is something that just makes things more
consistent and readable.
I don't see how this:
{'100755': 'x', '12': 'l'}
Is more readable than this:
{ '100755': 'x', '12': 'l' }
No strong opinion on this one though.
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.
Note that I'm not trying to yell, but these are just things that are set
forth in pep8 but don't seem to be set at all in git-remote-hg. I really
think that git's python 'bits' should be able to pass a default pep8
without issue.
And I disagree.
I'm willing to follow pep8 as much as possible as long as it makes
sense, but some thing do make the code uglier in my opinion.
That said there's a couple of issues present that I didn't notice.
Thanks for checking.
Hope to see some improvements! git-remote-hg is really quite useful for
me, and I hope it can be as good as possible!
Well, too bad Junio has essentially blocked all possible progress on his
tree.
I've pushed the fix on mine[1].
Cheers.
[1]
https://github.com/felipec/git/commit/12374c0e09a84945a202bb5ba7981a223d233d0b
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Conforming to pep8
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', '12': 'l'}
Is more readable than this:
{ '100755': 'x', '12': '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
pgp1iiH8t2JtT.pgp
Description: PGP signature
