On Tue, Jul 31, 2018 at 9:02 AM, Tim Armstrong < [email protected]> wrote:
> I agree that it might be a little strict at the moment and disallow some > reasonable formatting. The tool is controlled by the setup.cfg file in the > repo so it's easy enough to change the behaviour. > > I think we have been a little sloppy with Python style in general so I > think some of these would be good to change over time. I think the main > thing I'd like is to align the tool's behaviour with code reviews - if > we're going to be strict about PEP 8 compliance in code reviews we should > keep the tool strict. > > > 109 : E125 continuation line with same indent as next logical line > I think this formatting is hard to read and confusing, I'm in favour of > leaving this enabled. > > > 110 : E701 multiple statements on one line (colon) > This if because of the one-line if statements we have in the code. I don't > feel strongly either way as long as we're consistent. > > > 129 : E231 missing whitespace after ',' > > 185 : E251 unexpected spaces around keyword / parameter equals > > 288 : E502 the backslash is redundant between brackets > These seems like sloppy/inconsistent formatting to me, I'm in favour of > keeping these enabled and fixing existing code as we go. > > > 368 : E302 expected 2 blank lines, found 1 > Our Python code is very inconsistent here, would be nice to make it more > consistent. I'm in favour of keeping it enabled and fixing as we go. > > > 187 : E121 continuation line under-indented for hanging indent > > 1295 : E128 continuation line under-indented for visual indent > On one hand it would be nice to follow the PEP 8 style here ( > https://www.python.org/dev/peps/pep-0008/#indentation) but the other > idioms > seem fine to me. I've been asked on Python code reviews to switch to the > PEP 8 indentation style before so I think we should align the tools > behaviour with what we're going to ask for code reviews. > Alright, I agree with all of the above. One suggestion, though: is it possible to get something like 'autopep8' to run in a 'patch formatting' mode where it only reformats changed lines? Then we could more easily just run a single command to ensure that our patches are properly formatted before submitting to review. Or, at the very least, some instructions for running the same flake8-against-only-my-changed-lines that gerrit is running? -Todd > > > On Mon, Jul 30, 2018 at 7:48 PM, Todd Lipcon <[email protected]> > wrote: > > > It seems like flake8 might need some adjustment of its policies. Here are > > the most common issues in the current test code: > > > > 109 : E125 continuation line with same indent as next logical line > > 110 : E701 multiple statements on one line (colon) > > 129 : E231 missing whitespace after ',' > > 185 : E251 unexpected spaces around keyword / parameter equals > > 187 : E121 continuation line under-indented for hanging indent > > 288 : E502 the backslash is redundant between brackets > > 368 : E302 expected 2 blank lines, found 1 > > 1295 : E128 continuation line under-indented for visual indent > > > > Maybe worth just disabling some of the indentation-related ones to start? > > > > > > On Mon, Jul 30, 2018 at 4:09 PM, Tim Armstrong < > > [email protected]> wrote: > > > > > I have a few updates. > > > > > > I added an automatic build job for docs changes: > > > https://jenkins.impala.io/job/gerrit-docs-auto-test/ > > > > > > I'm going to disable the "Build started" message for > > > gerrit-code-review-checks. It seems a bit too chatty. Let me know if > you > > > disagree. > > > > > > I'm adding a job that automatically does some checks on the diff and > > posts > > > code review comments. I started off with Python flake8 comments. > > > > > > Let me know if you see any problems or if it turns out to be too noisy. > > > > > > > > > > > > On Mon, Jul 23, 2018 at 11:55 AM, Tim Armstrong < > [email protected] > > > > > > wrote: > > > > > > > Hi All, > > > > I'm enabling an automatic precommit job for code reviews uploaded > to > > > > gerrit that will run RAT, clang-tidy and a GCC debug compilation. > This > > is > > > > to provide faster feedback on code reviews: > https://issues.apache.org/ > > > > jira/browse/IMPALA-7317 . I'll add some more checks but I'm wanting > to > > > > test the basic mechanism for a bit now. > > > > > > > > It excludes docs/ commits. > > > > > > > > Let me know if you see any problems with it. > > > > > > > > Thanks, > > > > Tim > > > > > > > > > > > > > > > -- > > Todd Lipcon > > Software Engineer, Cloudera > > > -- Todd Lipcon Software Engineer, Cloudera
