-
Notifications
You must be signed in to change notification settings - Fork 653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
REFACTOR-#3900: add flake8-no-implicit-concat plugin and refactor flake8 error codes #3901
Conversation
c78bf7e
to
eca6307
Compare
Codecov Report
@@ Coverage Diff @@
## master #3901 +/- ##
==========================================
+ Coverage 86.71% 89.89% +3.18%
==========================================
Files 201 201
Lines 16800 16800
==========================================
+ Hits 14568 15103 +535
+ Misses 2232 1697 -535
Continue to review full report at Codecov.
|
Could you please try adding an implicit concat to this PR and checking whether the CI catches it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffreykennethli I see some failures in CI formatting.
@jeffreykennethli Line 24 in fea2064
We have to add a NIC warning type in the selection as well, to enable warnings from the plugin
|
Thanks @dchigarev. I added the NIC error codes and the errors show up now. I noticed we also have flake8-print installed but no error codes specified, so I'll add those too. |
scripts/test/examples.py
Outdated
@@ -15,6 +15,7 @@ | |||
# noqa: MD02 | |||
"""Function examples for docstring testing.""" | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this empty newline added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was formatted according to black's rules, see https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#empty-lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this change in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this change from this PR.
bfd1d3b
to
552adb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but do note that concatenating strings via +
has some performance penalties because this concatenation happens at runtime vs "implicit concatenation".
I think we can trade a bit of performance in exchange of better consistency here, though.
89404ce
d41b3a2
to
89404ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@modin-project/modin-omnisci This needs approval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a release note. Otherwise, LGTM!
0f486f8
to
366018b
Compare
…contributing docs Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
Signed-off-by: jeffreykennethli <jkli@ponder.io>
366018b
to
0868be1
Compare
What do these changes do?
To avoid errors like this one: https://codereviewdoctor.medium.com/5-of-666-python-repos-had-comma-typos-including-tensorflow-and-pytorch-sentry-and-v8-7bc3ad9a1bb7, add a recommendation to use flake8-no-implicit-concat in the contributing docs and add this plugin to the CI.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/developer/architecture.rst
is up-to-date