kmonsoor / pyglet

Automatically exported from code.google.com/p/pyglet
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

PEP-8 code style #733

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
I use PyDev with PEP-8 (http://legacy.python.org/dev/peps/pep-0008/) style 
checking and pyglet seems not to follow all its conventions. E.g. inline 
comments must be separated by at least 2 spaces, class or function definitions 
must be separated by 2 blank lines.
Definitions inside class use 1 blank line as separator.

You can use pep8.py to detect all problems.

Original issue reported on code.google.com by fen...@gmail.com on 12 Apr 2014 at 8:31

GoogleCodeExporter commented 9 years ago

Original comment by useboxnet on 12 Apr 2014 at 1:15

GoogleCodeExporter commented 9 years ago
I will try to have a look at this tonight in an effort to start contributing to 
Pyglet. It will be a good opportunity to get familiar with the code, and for 
some weird reason I like cleaning up code. I also love the PEP8 coding 
guidelines. I use flake8 as a tool, being that that also applies pyflakes.

Original comment by gwildorsok on 12 Apr 2014 at 3:50

GoogleCodeExporter commented 9 years ago
I've worked on this a bit, changes are on my clone:
https://code.google.com/r/gwildorsok-pyglet/source/list

Note that this issue is definitely not a one-man fix, because there are a lot 
of violations:

$ flake8 --count -qq contrib/
3442
$ flake8 --count -qq examples/
470
$ flake8 --count -qq experimental/
2997
$ flake8 --count -qq pyglet/
22099
$ flake8 --count -qq tools/
2156

Original comment by gwildorsok on 12 Apr 2014 at 9:43

GoogleCodeExporter commented 9 years ago
Starting long arguments list from a new line (instead of wrapping it between 
the brackets space) adds to readability, imho.

Original comment by Risimi...@gmail.com on 13 Apr 2014 at 9:03

Attachments:

GoogleCodeExporter commented 9 years ago
Generally I agree, although it depends on the situation. But PEP8 has no clear 
rule about that, and I didn't want to make any design decisions on this, which 
could higher the threshold to merge my changes, so I tried to be as agnostic as 
possible.

Original comment by gwildorsok on 13 Apr 2014 at 10:03

GoogleCodeExporter commented 9 years ago
I just pushed a commit to my clone which cleans up the examples module. What's 
left now are the big ones: contrib, experimental, the pyglet core itself, and 
tools. But I will wait with those until my current work has been merged, before 
it becomes "too big to merge" my changesets, or before the patches don't apply 
cleanly anymore.

Original comment by gwildorsok on 13 Apr 2014 at 4:38

GoogleCodeExporter commented 9 years ago
FYI you can use `autopep8` to make some progress toward PEP8 compliance. It 
misses some things, and I usually modify some of its changes, but all in all it 
can save *a lot* of time when converting to PEP8 compliance.

If people are going to be going through the codebase, would it be worth 
changing over to Py2/3 compatible code everywhere (e.g., `from __future__ 
import print_function` and `print()` statements)? Py3k PEP8 will eventually 
complain about these instances anyway (which you can silence, but why not fix 
them)...

Original comment by larson.e...@gmail.com on 16 Apr 2014 at 4:01

GoogleCodeExporter commented 9 years ago
Sanely speaking, yes, that would be a smart thing to do. I'm a bit concerned 
already, though, that the changes would become "too big to merge" and diverge 
too much unless it's done and coordinated in a proper manor. Also, that would 
bring design decisions, which would stall that process, increasing the risk of 
stranding the process.

Both reasons are also why I now deliberately wait until my changes have been 
merged upstream until I continue: I don't want this process to stall, so I try 
to do small parts at a time and wait until they have been merged to prevent my 
clone from diverging to much, and I avoid design decisions deliberately, as I'm 
not the person who should make those calls, and communicating about those 
decisions through Google Code or the mailing list takes too long, increasing 
the risk of this process end up being jammed.

Original comment by gwildorsok on 16 Apr 2014 at 7:15

GoogleCodeExporter commented 9 years ago
I agree that this "needs a plan", because making pyglet codebase pep8 will be a 
lot of work.

If you ask me, I don't think this is a priority (as any other thing that is not 
towards getting 1.2 our of the door!); but I don't want to put you off and it 
could be a great way of making easy tickets for newcomers.

I'm currently the only active committer doing actual work, so I'll try to help 
as much as I can; but my queue is already quite busy :(

My experience with other projects is that it is usually better if there are 
small tickets targeting one specific pep8 issue so they can be closed adding a 
simple patch that can be merged as easily as possible (as opposed to a big 
ticket that needs too much attention).

Thanks for your contribution :)

Original comment by useboxnet on 17 Apr 2014 at 5:51

GoogleCodeExporter commented 9 years ago
Ah, I'll take a look to your repo and see if I can start merging things; but I 
have other patches in my TODO that may make your merge not as easy to do.

Original comment by useboxnet on 17 Apr 2014 at 5:53

GoogleCodeExporter commented 9 years ago
That was also the reason I started on this ticket: getting to know the codebase 
of Pyglet a bit. Thanks to this ticket, I now know the test module quite well, 
although I danced around the core, so in that regard it did not help much. But 
at least I now have a working copy up and running.

On your view of small tickets and/or patches, I couldn't agree more.

Original comment by gwildorsok on 17 Apr 2014 at 5:37