Skip to content
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

Implement flake8-simplify #998

Closed
29 tasks done
charliermarsh opened this issue Dec 2, 2022 · 12 comments · Fixed by #2767
Closed
29 tasks done

Implement flake8-simplify #998

charliermarsh opened this issue Dec 2, 2022 · 12 comments · Fixed by #2767
Labels
good first issue Good for newcomers plugin Implementing a known but unsupported plugin

Comments

@charliermarsh
Copy link
Member

charliermarsh commented Dec 2, 2022

Python-specific rules:

  • SIM101 ("Multiple isinstance-calls which can be merged into a single call by using a tuple as a second argument")
  • SIM105 ("Use 'contextlib.suppress(...)' instead of try-except-pass")
  • SIM107 ("Don't use return in try/except and finally ")
  • SIM108 ("Use the ternary operator if it's reasonable ")
  • SIM109 ("Use a tuple to compare a value against multiple values")
  • SIM110 ("Use any(...) ")
  • SIM111 ("Use all(...)")
  • SIM113 ("Use enumerate instead of manually incrementing a counter")
  • SIM114 ("Combine conditions via a logical or to prevent duplicating code")
  • SIM115 ("Use context handler for opening files")
  • SIM116 ("Use a dictionary instead of many if/else equality checks")
  • SIM117 ("Merge with-statements that use the same scope")

Simplifying Comparisons:

  • SIM201 ("Use 'a != b' instead of 'not a == b'")
  • SIM202 ("Use 'a == b' instead of 'not a != b'")
  • SIM203 ("Use 'a not in b' instead of 'not (a in b)'")
  • SIM208 ("Use 'a' instead of 'not (not a)'")
  • SIM210 ("Use 'bool(a)' instead of 'True if a else False'")
  • SIM211 ("Use 'not a' instead of 'False if a else True'")
  • SIM212 ("Use 'a if a else b' instead of 'b if not a else a'")
  • SIM220 ("Use 'False' instead of 'a and not a'")
  • SIM221 ("Use 'True' instead of 'a or not a'")
  • SIM222 ("Use 'True' instead of '... or True'")
  • SIM223 ("Use 'False' instead of '... and False'")
  • SIM300 ("Use 'age == 42' instead of '42 == age'")

Simplifying usage of dictionaries:

  • SIM401 ("Use 'a_dict.get(key, "default_value")' instead of an if-block")
  • SIM118 ("Use 'key in dict' instead of 'key in dict.keys()'")

General Code Style:

  • SIM102 ("Use a single if-statement instead of nested if-statements")
  • SIM103 ("Return the boolean condition directly")
  • SIM112 ("Use CAPITAL environment variables")
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Dec 2, 2022
@charliermarsh
Copy link
Member Author

This plugin is particularly useful because we should be able to autofix most of these.

@charliermarsh
Copy link
Member Author

charliermarsh commented Jan 4, 2023

I'm working on SIM110 and SIM111.

(These actually require some weird logic whereby we need to be able to peek at the next sibling, so TBD.)

@danieleades
Copy link

looking good! I'll keep an eye on this issue and hopefully be able to remove the flake8-simplify plugin from Sphinx soon!

@charliermarsh
Copy link
Member Author

I'm working on SIM101.

charliermarsh added a commit that referenced this issue Jan 6, 2023
Flake8 simplify #998 

SIM201, SIM202 and SIM208 is done here with fixes.

Note: SIM203 == E713 

Co-authored-by: Charlie Marsh <[email protected]>
charliermarsh added a commit that referenced this issue Jan 7, 2023
@messense
Copy link
Contributor

messense commented Jan 8, 2023

SIM106 ("Handle error-cases first")

SIM106 was removed in flake8-simplify due to too many false-positives: MartinThoma/flake8-simplify#108

@charliermarsh
Copy link
Member Author

Removed, thanks!

charliermarsh pushed a commit that referenced this issue Jan 10, 2023
charliermarsh added a commit that referenced this issue Jan 12, 2023
charliermarsh pushed a commit that referenced this issue Jan 12, 2023
Ref #998 

- Implements SIM401 with fix
- Added tests

Notes: 
- only recognize simple ExprKind::Name variables in expr patterns for
now
- bug-fix from reference implementation: check 3-conditions (dict-key,
target-variable, dict-name) to be equal, `flake8_simplify` only test
first two (only first in second pattern)
@charliermarsh charliermarsh pinned this issue Jan 14, 2023
@charliermarsh charliermarsh unpinned this issue Jan 14, 2023
@jackklika
Copy link

jackklika commented Jan 20, 2023

FYI SIM103 autofix refactored my code to be incorrect:

(apps-z6RSF3Uo-py3.10) jack@macbook api % cat /tmp/a.py 
def verify_user(kind):
    if kind != "USER":
        return False
    else:
        return True
(apps-z6RSF3Uo-py3.10) jack@macbook api % ruff /tmp/a.py
/tmp/a.py:2:5: SIM103 Return the condition `kind != "USER"` directly
Found 1 error(s).
1 potentially fixable with the --fix option.
(apps-z6RSF3Uo-py3.10) jack@macbook api % ruff /tmp/a.py --fix
Found 1 error(s) (1 fixed, 0 remaining).
(apps-z6RSF3Uo-py3.10) jack@macbook api % cat /tmp/a.py      
def verify_user(kind):
    return kind != "USER"
(apps-z6RSF3Uo-py3.10) jack@macbook api % 

In the first case, the function returns False if the kind is not "USER", and True otherwise. In the fixed function, it returns the opposite. In the REPL:

>>> def verify_user(kind):
...     if kind != "USER":
...         return False
...     else:
...         return True
>>> verify_user("USER")
True
>>> def verify_user(kind):
...     return kind != "USER"
>>> verify_user("USER")
False

@charliermarsh
Copy link
Member Author

@jackklika - D'oh, thank you. I'll fix that this afternoon. Must be a flipped condition somewhere.

@harupy
Copy link
Contributor

harupy commented Jan 22, 2023

Can I implement SIM124?

https://github.com/MartinThoma/flake8-simplify#sim909 says:

The trial period starts on 28th of March and will end on 28th of September 2022.

@charliermarsh
Copy link
Member Author

Go for it!

@harupy
Copy link
Contributor

harupy commented Jan 22, 2023

@charliermarsh Is there a way to compare two expressions while ignoring their contexts?

foo = foo
^^^   ^^^
 |     |
 |      ---- Name { id: "foo", ctx: Load }
  ---------- Name { id: "foo", ctx: Store }

I tried ComparableExpr but it didn't work because the assignment target and value don't have the same context.

@charliermarsh
Copy link
Member Author

Hmm, no, we don't have anything to support that right now. Although arguably ComparableExpr should ignore ctx. Trying to think of where that would be incorrect.

charliermarsh added a commit that referenced this issue Jan 14, 2024
Implements SIM113 from #998

Added tests
Limitations 
   - No fix yet
   - Only flag cases where index variable immediately precede `for` loop

@charliermarsh please review and let me know any improvements

---------

Co-authored-by: Charlie Marsh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers plugin Implementing a known but unsupported plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants