Skip to content

top: Fix ruff rules: E721,F524,F541,F632 #10991

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 2, 2023
Merged

top: Fix ruff rules: E721,F524,F541,F632 #10991

merged 1 commit into from
May 2, 2023

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Mar 10, 2023

Fixes for:

  • type-comparison (E721)
    • Do not compare types, use isinstance()
  • string-dot-format-missing-arguments (F524)
    • .format call is missing argument(s) for placeholder(s): {message}
  • f-string-missing-placeholders (F541)
  • is-literal (F632)
    • Use == to compare constant literals
    • Use != to compare constant literals

When we have a pyproject.toml (#10977) we will need to ignore some test files that directly check these issues.

[tool.ruff.per-file-ignores]
"tests/basics/is_isnot_literal.py" = ["F632"]
"tests/basics/string_fstring.py" = ["F541"]
"tests/cpydiff/core_fstring_raw.py" = ["F541"]

@github-actions
Copy link

github-actions bot commented Mar 10, 2023 •
edited
Loading

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    -4 -0.001% PYBV10
        rp2:    +0 +0.000% PICO

@codecov-commenter
Copy link

Codecov Report

Merging #10991 (fed237f) into master (4376c96) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master   #10991   +/-   ##
=======================================
  Coverage   98.50%   98.50%           
=======================================
  Files         155      155           
  Lines       20549    20549           
=======================================
  Hits        20241    20241           
  Misses        308      308

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cclauss cclauss mentioned this pull request Mar 10, 2023
Merged
@cclauss cclauss force-pushed the ruff_--fix_f-string_without_placeholders branch from fed237f to 22bf8fd Compare March 10, 2023 10:11
@cclauss cclauss changed the title Fix ruff rules: E721,F524,F541,F632 top: Fix ruff rules: E721,F524,F541,F632 Mar 10, 2023
dpgeorge
dpgeorge reviewed May 2, 2023
View reviewed changes
extmod/uasyncio/funcs.py Outdated
@@ -123,7 +123,7 @@ def done(t, er):

# Either this gather was cancelled, or one of the sub-tasks raised an exception with
# return_exceptions==False, so reraise the exception here.
if state is not 0:
if state != 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state can be an integer or an exception type, so that's why is not is used

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is not 0 only works because of the implementation detail of MP_SMALL_INT but in general is should not be used with ints.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right. So I'll need to check in detail what happens for state != 0 when state is an exception instance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't if state: work since 0 is falsy but any exception will be truthy?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's a good idea. At this point state is either 0 or an exception instance (because in the latter case it's raising it).

shared/memzip/make-memzip.py Outdated
@@ -36,7 +36,7 @@ def create_c_from_file(c_filename, zip_filename):
break
print(' ', end='', file=c_file)
for byte in buf:
if type(byte) is types.StringType:
if type(byte) is types.StringType: # noqa: E721
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this can be changed to use isinstance?

@cclauss cclauss force-pushed the ruff_--fix_f-string_without_placeholders branch from 22bf8fd to 86f3320 Compare May 2, 2023 05:54
@cclauss cclauss requested review from dlech and dpgeorge May 2, 2023 05:54
@cclauss cclauss force-pushed the ruff_--fix_f-string_without_placeholders branch from 86f3320 to 97de2db Compare May 2, 2023 05:55
@cclauss @dpgeorge
all: Fix various Python coding inconsistencies found by ruff.
79e5747 
This fixes:
- type-comparison (E721): do not compare types, use isinstance().
- string-dot-format-missing-arguments (F524): .format call is missing
  argument(s) for placeholder(s): {message}.
- f-string-missing-placeholders (F541).
- is-literal (F632): Use != to compare constant literals.

The last one is fixed by just comparing for truthfulness of `state`.
@dpgeorge dpgeorge force-pushed the ruff_--fix_f-string_without_placeholders branch from 97de2db to 79e5747 Compare May 2, 2023 06:19
@dpgeorge
Copy link
Member

dpgeorge commented May 2, 2023

I updated the commit message with more detail.

@dpgeorge dpgeorge merged commit 79e5747 into micropython:master May 2, 2023
@dpgeorge
Copy link
Member

dpgeorge commented May 2, 2023

Thanks for the fixes!

@cclauss cclauss deleted the ruff_--fix_f-string_without_placeholders branch May 2, 2023 06:32
@cclauss
Copy link
Contributor Author

cclauss commented May 2, 2023 •
edited
Loading

I updated the commit message with more detail.

As an alternative, you can also do ruff rule E721 which will kick out markdown text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge dpgeorge approved these changes

@dlech dlech Awaiting requested review from dlech

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cclauss @codecov-commenter @dpgeorge @dlech