You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Laszlo Gaal (Code Review)" <ge...@cloudera.org> on 2025/02/06 16:17:46 UTC
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Laszlo Gaal has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22460
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
IMPALA-13741: Ignore flake8 W504 during precommit quick checks
bin/jenkins/critique-gerrit-review.py warns about W504:"line break after
binary operator" during first-pass precommit checks for new patches
submitted for Impala on gerrit.cloudera.org.
W504 was created in April 2016, when flake8 changed their opinion about
the way line breaks should be handled around binary operators: the
accepted way before that date was exactly the opposite.
Since the majority of Python code in Impala was created before that
date, existing Impala convention is to add the line break after the
operator, so W504 does not make sense for Impala.
For this reason, this patch disables W504 for flake8 via a command-line
parameter passed to flake8 from critique-gerrit-review.py
Tested locally.
Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/22460/1
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 1:
(1 comment)
I generally agree with disabling this check since it contradicts so much of our existing convention.
http://gerrit.cloudera.org:8080/#/c/22460/1/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22460/1/bin/jenkins/critique-gerrit-review.py@55
PS1, Line 55: FLAKE8_IGNORE_W504 = "--ignore=W504"
I think it'd be better to do this as a flake8 config file - https://flake8.pycqa.org/en/latest/user/configuration.html - so it applies to developers doing local checks as well.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2025 16:44:40 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/18216/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2025 16:45:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Laszlo Gaal (Code Review)" <ge...@cloudera.org>.
Laszlo Gaal has abandoned this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Abandoned
Sticking to the original decision to migrate gradually to the new PEP8 style.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/18219/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2025 17:24:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py@55
PS2, Line 55: FLAKE8_IGNORE_W504 = "--ignore=W504"
Why this is not set in https://github.com/apache/impala/blob/master/setup.cfg ?
This conflict with the opposite ignore at https://www.flake8rules.com/rules/W503.html (0f6d051 - Ignore flake8 W503 about breaking before operators).
One of them must give up.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 15:59:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 2: Code-Review-1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py@55
PS2, Line 55: FLAKE8_IGNORE_W504 = "--ignore=W504"
> Why this is not set in https://github.com/apache/impala/blob/master/setup.c
I forgot we have prior art stating we should slowly migrate to the new rule. Let's stick with that direction.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Tue, 11 Feb 2025 17:35:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/18223/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2025 17:42:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Laszlo Gaal (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22460
to look at the new patch set (#2).
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
IMPALA-13741: Ignore flake8 W504 during precommit quick checks
bin/jenkins/critique-gerrit-review.py warns about W504:"line break after
binary operator" during first-pass precommit checks for new patches
submitted for Impala on gerrit.cloudera.org.
W504 was created in April 2016, when flake8 changed their opinion about
the way line breaks should be handled around binary operators: the
accepted way before that date was exactly the opposite.
Since the majority of Python code in Impala was created before that
date, existing Impala convention is to add the line break after the
operator, so W504 does not make sense for Impala.
For this reason, this patch disables W504 for flake8 via a command-line
parameter passed to flake8 from critique-gerrit-review.py
Tested locally.
Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
---
M bin/jenkins/critique-gerrit-review.py
1 file changed, 3 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/22460/2
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-13741: Ignore flake8 W504 during precommit quick checks
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22460 )
Change subject: IMPALA-13741: Ignore flake8 W504 during precommit quick checks
......................................................................
Patch Set 2:
(1 comment)
gerrit-auto-critic failed. You can reproduce it locally using command:
python3 bin/jenkins/critique-gerrit-review.py --dryrun
To run it, you might need a virtual env with Python3's venv installed.
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py
File bin/jenkins/critique-gerrit-review.py:
http://gerrit.cloudera.org:8080/#/c/22460/2/bin/jenkins/critique-gerrit-review.py@162
PS2, Line 162: [
flake8: E126 continuation line over-indented for hanging indent
--
To view, visit http://gerrit.cloudera.org:8080/22460
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id118c4a92dcce1b7a315f85c2bcc9afdd5b6b886
Gerrit-Change-Number: 22460
Gerrit-PatchSet: 2
Gerrit-Owner: Laszlo Gaal <la...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 06 Feb 2025 17:24:10 +0000
Gerrit-HasComments: Yes