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

Enable ruff's flake8-return rule #3047

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

CoolCat467
Copy link
Member

In this pull request, we enable ruff's flake8-return rule and fix all the new issues from doing this.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 97.22222% with 3 lines in your changes missing coverage. Please review.

Project coverage is 99.58%. Comparing base (af010af) to head (2fdf4af).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
src/trio/_core/_io_kqueue.py 0.00% 1 Missing ⚠️
src/trio/_dtls.py 85.71% 1 Missing ⚠️
src/trio/_tests/test_subprocess.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3047      +/-   ##
==========================================
- Coverage   99.58%   99.58%   -0.01%     
==========================================
  Files         121      121              
  Lines       18167    18158       -9     
  Branches     3268     3268              
==========================================
- Hits        18092    18083       -9     
  Misses         52       52              
  Partials       23       23              
Files with missing lines Coverage Δ
src/trio/_core/_ki.py 100.00% <100.00%> (ø)
src/trio/_core/_mock_clock.py 100.00% <100.00%> (ø)
src/trio/_core/_run.py 99.02% <100.00%> (ø)
src/trio/_core/_tests/test_guest_mode.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_run.py 100.00% <ø> (ø)
src/trio/_core/_tests/test_thread_cache.py 100.00% <100.00%> (ø)
src/trio/_core/_tests/test_windows.py 100.00% <100.00%> (ø)
src/trio/_core/_unbounded_queue.py 100.00% <100.00%> (ø)
src/trio/_file_io.py 100.00% <100.00%> (ø)
src/trio/_highlevel_generic.py 100.00% <100.00%> (ø)
... and 23 more

@TeamSpen210
Copy link
Contributor

Not sure if this is worth it, it just causes a lot of churn indenting/dedenting things for no real reason. The elif technically is redundant if the previous if always returns, but it makes it more clear that they're mutually exclusive, and might cause bugs in future if someone removes one of the returns. This also causes symmetric if-else checks to become non-symmetric now, which might be confusing to the reader.

@CoolCat467 CoolCat467 added the skip newsfragment Newsfragment is not required label Aug 21, 2024
@A5rocks
Copy link
Contributor

A5rocks commented Aug 26, 2024

I agree with @TeamSpen210 that this doesn't really help anything. It doesn't make things clearer (unlike flake8-commas) and removes semantics. Looking through the error codes listed on https://pypi.org/project/flake8-return/ (I'm sure ruff implements them smarter, ofc):

  • R501 conflicts with mypy nvm, I was confusing this with implicit None return w/ a return type like None | int. I guess this is useful for standardization reasons, but I don't really like it.
  • R502/R503 is just type checking
  • R504 is potentially useful, but in general assigning to a variable is useful because it allows us to show a nicer name in tracebacks. (maybe we could just enable this? I'm not too sure)
  • R505/R506/R507/R508 remove semantics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip newsfragment Newsfragment is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants