Hi,
I might have ran into a bug of gerrit-code-review-checks with this change:
https://gerrit.cloudera.org/#/c/11160/
The job failed for PS 7, because it didn't contain
bin/jenkins/build-only.sh.
So I rebased and uploaded PS 8, but that didn't trigger
gerrit-code-review-checks. So I went to jenkins and hit "Retrigger" on the
failed job, but it executed the job against PS 7, not PS 8.
And when I look at "Build with parameters", I can only see the
IMPALA_REPO_URL and IMPALA_LZO_BRANCH parameters, so I cannot really run
this job against the my newest patch set.
Anyway the change is already +2ed and I could run gerrit-verify-dryrun
which almost certainly do these checks as well, but if it didn't I don't
want to commit something that might break gerrit-core-review-checks for
subsequent changes.
Thanks,
Zoltan
On Wed, Aug 1, 2018 at 10:56 PM Tim Armstrong
<[email protected]> wrote:
> The flake8 comments should now line up correctly with your diff. I got more
> feedback that some of the comments were a little strict (and it's unclear
> which ones will be enforced) so I posted a diff to disable some of the
> checks:
>
> https://gerrit.cloudera.org/#/c/11102/
>
> On Tue, Jul 31, 2018 at 10:00 AM, Tim Armstrong <[email protected]>
> wrote:
>
> > Todd pointed out a bug where it was posting flake8 comments that didn't
> > align with the diff. I figured out the issue but will keep the job in a
> > silent mode for a bit while I monitor it.
> >
> > On Tue, Jul 31, 2018 at 9:16 AM, Jim Apple <[email protected]
> >
> > wrote:
> >
> >> This is from my .emacs. The way it works is that you select a region and
> >> then M-x pep8-region.
> >>
> >> (defun pep8-region (&optional b e)
> >> (interactive "r")
> >> (call-process-region b e
> >> "/home/jbapple/.local/bin/autopep8" t t nil
> >> "--indent-size=2" "--max-line-length=90" "-a"
> "-a"
> >> "-a" "-a" "-a" "-a" "-a" "-a" "--experimental"
> >> "-"))
> >>
> >> It's . . . not great. For instance, it doesn't understand that if you're
> >> already 4 spaces in at the start of the region, you don't want to revert
> >> back to 0 or 2 spaces in. That said, it can still be helpful. I don't
> >> think
> >> this is as sophisticated as clang-format.el.
> >>
> >> On Tue, Jul 31, 2018 at 9:09 AM, Todd Lipcon <[email protected]
> >
> >> wrote:
> >>
> >> > 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
> >> >
> >>
> >
> >
>
