impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Lars Volker ...@cloudera.com>
Subject Re: Python string formatting
Date Wed, 01 Feb 2017 16:26:11 GMT
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 <jbapple@cloudera.com> 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 <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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message