Thanks for the feedback. Printing all tests and making sure the names stay the same is a very good idea.
I also just stumbled over yapf (https://github.com/google/yapf) which is a python code formatter based on clang-format's logic. I will check that one out too and try to come up with a patch for review when I have some spare time. On Wed, Feb 1, 2017 at 3:33 AM, Jim Apple <[email protected]> wrote: > 1. Autopep8 might break a part of a test that is rarely run, so even > if exhaustive works, something may have gotten messed up. > > 2. David pointed out that you can print the tests that are scheduled > to be run by using pytest's dry-run option. You can then check that > the number of tests (and maybe even the configuration and names of > them?) stay the same after autopep8/ > > 3. You could use https://docs.python.org/2/library/ast.html to make > sure that only the whitespace is being changed and the AST is staying > the same. That might be overkill. > > On Tue, Jan 31, 2017 at 5:53 PM, Lars Volker <[email protected]> wrote: > > Thanks for pointing this out. Yes, I thought of trying autopep8 and > running > > an exhaustive build. I assumed it wouldn't break tests, but that is > > probably a naive assumption. However, if nothing breaks in an exhaustive > > run I'd be quite confident that nothing else will. That leaves the risk > > that a change made by autopep8 disables a test so we don't notice the > > breakage. Other than manually reviewing the code I don't have an idea how > > to prevent that. > > > > Thoughts? > > > > On Tue, Jan 31, 2017 at 8:29 PM, Jim Apple <[email protected]> wrote: > > > >> Will you use autopep8? If so, how will you check that it doesn't break > >> something on an infrequently-used codepath? > >> > >> On Tue, Jan 31, 2017 at 11:12 AM, Michael Brown <[email protected]> > >> wrote: > >> > I agree. > >> > > >> > On Tue, Jan 31, 2017 at 10:44 AM, Lars Volker <[email protected]> > wrote: > >> >> Thanks for the feedback here and in the review. > >> >> > >> >> I agree that we should aim for a style as close to PEP8 as possible. > >> >> However, I also didn't want to overshoot and my first goal was to get > >> some > >> >> useful tooling set up, so that I don't have to constantly worry about > >> >> formatting. Once I had figured out some tooling, I thought I might as > >> well > >> >> share it and solicit feedback. > >> >> > >> >> Regarding the next steps, I'm open for anything really. I didn't know > >> about > >> >> the --diff switch of flake8, that looks very useful. Even better of > >> course > >> >> would be, if all python code could be converted to PEP8. > >> >> > >> >> Here is a list of all PEP8 violations and their count, obtained with > >> >> "pycodestyle --statistics -qq tests": > >> >> > >> >> 9017 E111 indentation is not a multiple of four > >> >> 902 E114 indentation is not a multiple of four (comment) > >> >> 2 E116 unexpected indentation (comment) > >> >> 24 E122 continuation line missing indentation or outdented > >> >> 5 E124 closing bracket does not match visual indentation > >> >> 105 E125 continuation line with same indent as next logical line > >> >> 43 E127 continuation line over-indented for visual indent > >> >> 1038 E128 continuation line under-indented for visual indent > >> >> 7 E131 continuation line unaligned for hanging indent > >> >> 13 E201 whitespace after '(' > >> >> 8 E202 whitespace before ']' > >> >> 55 E203 whitespace before ':' > >> >> 5 E211 whitespace before '[' > >> >> 5 E221 multiple spaces before operator > >> >> 7 E222 multiple spaces after operator > >> >> 9 E225 missing whitespace around operator > >> >> 1 E227 missing whitespace around bitwise or shift operator > >> >> 127 E231 missing whitespace after ':' > >> >> 157 E251 unexpected spaces around keyword / parameter equals > >> >> 20 E261 at least two spaces before inline comment > >> >> 21 E265 block comment should start with '# ' > >> >> 1 E266 too many leading '#' for block comment > >> >> 1 E271 multiple spaces after keyword > >> >> 4 E301 expected 1 blank line, found 0 > >> >> 313 E302 expected 2 blank lines, found 1 > >> >> 16 E303 too many blank lines (2) > >> >> 13 E305 expected 2 blank lines after class or function > definition, > >> >> found 1 > >> >> 6 E306 expected 1 blank line before a nested definition, found > 0 > >> >> 7 E402 module level import not at top of file > >> >> 3800 E501 line too long (80 > 79 characters) > >> >> 278 E502 the backslash is redundant between brackets > >> >> 87 E701 multiple statements on one line (colon) > >> >> 74 E703 statement ends with a semicolon > >> >> 12 E711 comparison to None should be 'if cond is None:' > >> >> 9 E712 comparison to False should be 'if cond is False:' or 'if > >> not > >> >> cond:' > >> >> 2 E713 test for membership should be 'not in' > >> >> 2 E741 ambiguous variable name 'l' > >> >> 1 W292 no newline at end of file > >> >> 9 W391 blank line at end of file > >> >> 2 W601 .has_key() is deprecated, use 'in' > >> >> 19 W602 deprecated form of raising exception > >> >> > >> >> If we take out the well known ones (indent, line width), it does not > >> look > >> >> too far fetched to me to change it all to PEP8. > >> >> > >> >> Thoughts? > >> >> > >> >> > >> >> > >> >> On Tue, Jan 31, 2017 at 5:59 PM, Michael Brown <[email protected]> > >> wrote: > >> >> > >> >>> Thanks. I made some comments on the review, but I see now I should > >> >>> probably share my general view here. > >> >>> > >> >>> My general view is, if we are going to codify our Python style > guide, > >> >>> I would rather we codify style conventions that are closer to > standard > >> >>> Python style conventions, rather than codify what is currently > done. I > >> >>> am willing to keep 2-space indents and 90-char lines, but I don't > >> >>> think anything else should be part of the conventions when those > >> >>> conventions involves ignoring PEP-008. My instinct tells me the > Python > >> >>> conventions weren't conventions at all, but came up organically > >> >>> without regard to actually reading conventions or using tooling. > >> >>> Otherwise, we'd have already had a Python style guide, right? > >> >>> > >> >>> If the concern is "But there are too many noisy errors if I am > editing > >> >>> an existing, large file, so we should ignore these anyway", > something > >> >>> like this is possible: > >> >>> > >> >>> git diff | flake8 --diff > >> >>> > >> >>> This will only show PEP-008 problems on changed code, not whole > files. > >> >>> > >> >>> > >> >>> > >> >>> On Mon, Jan 30, 2017 at 3:20 PM, Lars Volker <[email protected]> > wrote: > >> >>> > Cool, thanks Michael for the reply. I added a section on Python to > >> the > >> >>> Impala > >> >>> > Style Guide > >> >>> > <https://cwiki.apache.org/confluence/display/IMPALA/ > >> Impala+Style+Guide>. > >> >>> > Please feel free to edit it or let me know if I should make > changes. > >> I > >> >>> will > >> >>> > also send out a review to add a .pep8rc file to the repository. > >> >>> > > >> >>> > On Fri, Jan 27, 2017 at 11:56 PM, Michael Brown < > [email protected]> > >> >>> wrote: > >> >>> > > >> >>> >> I prefer str.format() over the % operator, because: > >> >>> >> > >> >>> >> https://docs.python.org/2.7/library/stdtypes.html#str.format > >> >>> >> > >> >>> >> "This method of string formatting is the new standard in Python > 3, > >> and > >> >>> >> should be preferred to the % formatting described in String > >> Formatting > >> >>> >> Operations in new code." > >> >>> >> > >> >>> >> Without an Impala Python style guide, I tend to use what I see on > >> >>> >> docs.python.org, modulo our 2-space indent and 90-char line > policy. > >> >>> >> > >> >>> >> > >> >>> >> On Fri, Jan 27, 2017 at 2:44 PM, Lars Volker <[email protected]> > >> wrote: > >> >>> >> > Hi All, > >> >>> >> > > >> >>> >> > do we have a strong preference for either old style or new > style > >> >>> string > >> >>> >> > formatting in Python? > >> >>> >> > > >> >>> >> > "Hello %s!" % ("world") *vs* "Hello {0}!".format("world") > >> >>> >> > > >> >>> >> > The Impala Style Guide > >> >>> >> > <https://cwiki.apache.org/confluence/display/IMPALA/ > >> >>> Impala+Style+Guide> > >> >>> >> doesn't > >> >>> >> > mention Python at all. > >> >>> >> > > >> >>> >> > Thanks, Lars > >> >>> >> > >> >>> > >> >
