impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jim Apple <jbap...@cloudera.com>
Subject Re: Python string formatting
Date Wed, 01 Feb 2017 02:33:10 GMT
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 <lv@cloudera.com> 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 <jbapple@cloudera.com> 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 <mikeb@cloudera.com>
>> wrote:
>> > I agree.
>> >
>> > On Tue, Jan 31, 2017 at 10:44 AM, Lars Volker <lv@cloudera.com> 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 <mikeb@cloudera.com>
>> 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 <lv@cloudera.com>
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 <mikeb@cloudera.com>
>> >>> 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 <lv@cloudera.com>
>> 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
>> >>> >>
>> >>>
>>

Mime
View raw message