djc / rnc2rng

RELAX NG Compact to regular syntax conversion library
MIT License
16 stars 13 forks source link

Fix flakes #5

Closed jayvdb closed 8 years ago

jayvdb commented 8 years ago

If you're not happy with any of this, I am happy to look for alternatives, ignore certain flake8 codes, or even whitelist certain lines using my own beast https://pypi.python.org/pypi/flake8-putty .

djc commented 8 years ago

Hey, thanks for taking a look. I think pyflakes is really useful in finding issues with the code, but with pep8 stuff I generally feel that I keep my code clean enough and that the automatic stuff isn't really a win. So I would prefer not to add any automatic PEP 8 checking, and am not that interested in most of the changes here. Some things I'd be happy to accept (ideally in separate commits):

I'd be happy if you want to pick those up, but otherwise I can do it myself. :)

jayvdb commented 8 years ago

No worries; I'll adjust it. Thx for the feedback.

jayvdb commented 8 years ago

I've ignored the errors pep8 was raising.

With regards to "[moving] globals assignment right after the definition", I've moved it, but I do not agree. Placing them near the top means latter code in the module (added in the future) could overwrite them. The assert helps catch that, but the asserts are only effective if there is no other code after it. An alternative is to add a unit test to assert that the defines haven't been altered during module loading.

Before splitting it off into separate commits, I'd like to reply to your feedback re pep8, hoping to convince you to allow flake8 & pep8 checking, rather than just pyflakes checking.

As you can see from the modified diff, the code is already mostly pep8 compliant, with only minor violations. I agree you are keeping your code clean. The problem is that other people may not be as clean as yourself, especially if you do not signal your code quality expectations before they submit a PR. By including flake8 in .travis.yml or tox.ini, potential submitters have a good idea about how to prepare their patch.

flake8 adds # noqa support, which is a very handy way to bypass pyflakes bugs. I avoids adding dead code. I am a pyflakes developer, and some of the pyflakes bugs you may encounter in the future are not going to be easily fixed in pyflakes; workarounds are needed.

jayvdb commented 8 years ago

Also flake8 has a number of extensions which can help catch silly mistakes, or very poor style. If you really don't want pep8 running, we can disable all pep8 rules by ignoring 'E,W'

djc commented 8 years ago

Hey, good job arguing for this! The current change is looking much better already...

I kind of like the idea of a unittest to test that the node type constant values are correct. The reason I don't really want it at the bottom is that it seems like one way of having spooky action at a distance. Also, they way they're being used (with string literals used in the parser functions and the constants only being used in the generator), I think with the high coverage, all problems with this will already lead to test failures.

djc commented 8 years ago

Awesome, thanks for all the work!