From: William Giokas <hidden> Date: 2016-06-15 23:01:04
Hi all,
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::
./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
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'
1022 W191 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()'
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.
Thanks,
--
William Giokas | KaiSforza | http://kaictl.net/
GnuPG Key: 0x73CD09CF
Fingerprint: F73F 50EF BBE2 9846 8306 E6B8 6902 06D8 73CD 09CF
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:04
(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'
1022 W191 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()'
From: Felipe Contreras <hidden> Date: 2016-06-15 23:01:04
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
From: William Giokas <hidden> Date: 2016-06-15 23:01:04
On Thu, May 08, 2014 at 09:10:25PM -0500, Felipe Contreras wrote:
William Giokas wrote:
quoted
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
From: Felipe Contreras <hidden> Date: 2016-06-15 23:01:04
William Giokas wrote:
On Thu, May 08, 2014 at 09:10:25PM -0500, Felipe Contreras wrote:
quoted
William Giokas wrote:
quoted
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', '120000': 'l'}
Is more readable than this:
{ '100755': 'x', '120000': '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.
quoted
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.
quoted
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!
From: William Giokas <hidden> Date: 2016-06-15 23:01:04
On Thu, May 08, 2014 at 11:36:29PM -0500, Felipe Contreras wrote:
William Giokas wrote:
quoted
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.
quoted
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.
quoted
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].
quoted
E12{6,8}: continuation line indenting is another thing that helps
consistency.
I don't see how.
quoted
quoted
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
From: Felipe Contreras <hidden> Date: 2016-06-15 23:01:04
William Giokas wrote:
On Thu, May 08, 2014 at 11:36:29PM -0500, Felipe Contreras wrote:
quoted
William Giokas wrote:
quoted
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.
quoted
quoted
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.
quoted
quoted
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].
I don't see inconsistency within the script. All the hashes are in the
form of { content }.
quoted
quoted
quoted
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
From: William Giokas <hidden> Date: 2016-06-15 23:01:04
On Fri, May 09, 2014 at 02:18:54AM -0500, Felipe Contreras wrote:
William Giokas wrote:
quoted
On Thu, May 08, 2014 at 11:36:29PM -0500, Felipe Contreras wrote:
quoted
William Giokas wrote:
quoted
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
From: Felipe Contreras <hidden> Date: 2016-06-15 23:01:04
William Giokas wrote:
On Fri, May 09, 2014 at 02:18:54AM -0500, Felipe Contreras wrote:
quoted
William Giokas wrote:
quoted
On Thu, May 08, 2014 at 11:36:29PM -0500, Felipe Contreras wrote:
quoted
William Giokas wrote:
quoted
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
From: William Giokas <hidden> Date: 2016-06-15 23:01:04
On Fri, May 09, 2014 at 02:35:34AM -0500, Felipe Contreras wrote:
William Giokas wrote:
quoted
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
From: John Keeping <hidden> Date: 2016-06-15 23:01:04
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.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:05
On 05/09/2014 04:09 AM, Jonathan Nieder wrote:
(cc-ing Pete Wyckoff who maintains git-p4 and Michael Haggerty
who maintains git-multimail)
William Giokas wrote:
quoted
- 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.
[...]
Thanks for the heads-up, William and Jonathan. I wasn't aware of the
pep8 tool. Though I think it enforces a couple of rules that go a bit
beyond PEP8, I don't mind conforming to it, at least with options
--max-line-length=99 --hang-closing
I just committed a change to my upstream repository that gets pep8 to
run cleanly on all Python files.
I don't think it is worth making an extra code drop to Git just to push
these mild stylistic changes, but if somebody disagrees you know where
to find my repo.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: W. Trevor King <hidden> Date: 2016-06-15 23:01:05
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