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

[flake8-return] - Add fixes for (RET505, RET506, RET507, RET508) #9595

Merged

Conversation

diceroll123
Copy link
Contributor

Summary

add fix for RET505, RET506, RET507, RET508

Test Plan

cargo test

Copy link

codspeed-hq bot commented Jan 21, 2024

CodSpeed Performance Report

Merging #9595 will degrade performances by 22.56%

Comparing diceroll123:add-fixes-to-RET505,506,507,508 (c89ce6f) with main (a42600e)

Summary

❌ 2 (👁 2) regressions
✅ 28 untouched benchmarks

Benchmarks breakdown

Benchmark main diceroll123:add-fixes-to-RET505,506,507,508 Change
👁 linter/all-with-preview-rules[pydantic/types.py] 56.8 ms 64.5 ms -11.94%
👁 linter/all-with-preview-rules[numpy/ctypeslib.py] 26.6 ms 34.4 ms -22.56%

@diceroll123 diceroll123 marked this pull request as draft January 21, 2024 07:19
Copy link
Contributor

github-actions bot commented Jan 21, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +1884 -0 fixes in 4 projects; 39 projects unchanged)

apache/airflow (+0 -0 violations, +1118 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- airflow/__init__.py:95:5: RET506 Unnecessary `elif` after `raise` statement
+ airflow/__init__.py:95:5: RET506 [*] Unnecessary `elif` after `raise` statement
- airflow/api_connexion/endpoints/config_endpoint.py:119:5: RET505 Unnecessary `elif` after `return` statement
+ airflow/api_connexion/endpoints/config_endpoint.py:119:5: RET505 [*] Unnecessary `elif` after `return` statement
- airflow/api_connexion/endpoints/config_endpoint.py:85:5: RET505 Unnecessary `elif` after `return` statement
+ airflow/api_connexion/endpoints/config_endpoint.py:85:5: RET505 [*] Unnecessary `elif` after `return` statement
- airflow/api_connexion/endpoints/dag_run_endpoint.py:429:5: RET505 Unnecessary `else` after `return` statement
+ airflow/api_connexion/endpoints/dag_run_endpoint.py:429:5: RET505 [*] Unnecessary `else` after `return` statement
- airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:47:9: RET506 Unnecessary `else` after `raise` statement
+ airflow/api_connexion/endpoints/forward_to_fab_endpoint.py:47:9: RET506 [*] Unnecessary `else` after `raise` statement
- airflow/api_connexion/schemas/common_schema.py:122:9: RET505 Unnecessary `elif` after `return` statement
+ airflow/api_connexion/schemas/common_schema.py:122:9: RET505 [*] Unnecessary `elif` after `return` statement
... 815 additional changes omitted for rule RET505
- airflow/cli/commands/dag_command.py:237:5: RET506 Unnecessary `elif` after `raise` statement
+ airflow/cli/commands/dag_command.py:237:5: RET506 [*] Unnecessary `elif` after `raise` statement
- airflow/cli/commands/dag_command.py:258:5: RET506 Unnecessary `elif` after `raise` statement
+ airflow/cli/commands/dag_command.py:258:5: RET506 [*] Unnecessary `elif` after `raise` statement
... 267 additional changes omitted for rule RET506
- airflow/dag_processing/manager.py:567:17: RET508 Unnecessary `elif` after `break` statement
+ airflow/dag_processing/manager.py:567:17: RET508 [*] Unnecessary `elif` after `break` statement
- airflow/providers/amazon/aws/hooks/sagemaker.py:1164:21: RET508 Unnecessary `else` after `break` statement
+ airflow/providers/amazon/aws/hooks/sagemaker.py:1164:21: RET508 [*] Unnecessary `else` after `break` statement
- airflow/providers/amazon/aws/secrets/secrets_manager.py:232:13: RET507 Unnecessary `elif` after `continue` statement
+ airflow/providers/amazon/aws/secrets/secrets_manager.py:232:13: RET507 [*] Unnecessary `elif` after `continue` statement
- airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:625:13: RET507 Unnecessary `elif` after `continue` statement
+ airflow/providers/cncf/kubernetes/executors/kubernetes_executor.py:625:13: RET507 [*] Unnecessary `elif` after `continue` statement
- airflow/providers/snowflake/operators/snowflake.py:523:13: RET508 Unnecessary `elif` after `break` statement
+ airflow/providers/snowflake/operators/snowflake.py:523:13: RET508 [*] Unnecessary `elif` after `break` statement
- airflow/serialization/serialized_objects.py:982:13: RET507 Unnecessary `elif` after `continue` statement
+ airflow/serialization/serialized_objects.py:982:13: RET507 [*] Unnecessary `elif` after `continue` statement
- airflow/utils/helpers.py:349:13: RET507 Unnecessary `elif` after `continue` statement
... 1089 additional changes omitted for project

bokeh/bokeh (+0 -0 violations, +306 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- examples/server/app/clustering/main.py:88:5: RET505 Unnecessary `elif` after `return` statement
+ examples/server/app/clustering/main.py:88:5: RET505 [*] Unnecessary `elif` after `return` statement
- release/checks.py:105:9: RET505 Unnecessary `else` after `return` statement
+ release/checks.py:105:9: RET505 [*] Unnecessary `else` after `return` statement
- release/checks.py:119:9: RET505 Unnecessary `else` after `return` statement
+ release/checks.py:119:9: RET505 [*] Unnecessary `else` after `return` statement
... 239 additional changes omitted for rule RET505
- src/bokeh/application/application.py:169:9: RET506 Unnecessary `elif` after `raise` statement
+ src/bokeh/application/application.py:169:9: RET506 [*] Unnecessary `elif` after `raise` statement
- src/bokeh/application/handlers/directory.py:154:9: RET506 Unnecessary `elif` after `raise` statement
+ src/bokeh/application/handlers/directory.py:154:9: RET506 [*] Unnecessary `elif` after `raise` statement
... 296 additional changes omitted for project

demisto/content (+0 -0 violations, +38 -0 fixes)

ruff check --no-cache --exit-zero --preview

- Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:383:13: RET508 Unnecessary `elif` after `break` statement
+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:383:13: RET508 [*] Unnecessary `elif` after `break` statement
- Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:501:13: RET508 Unnecessary `elif` after `break` statement
+ Packs/ARIAPacketIntelligence/Integrations/ARIAPacketIntelligence/ARIAPacketIntelligence.py:501:13: RET508 [*] Unnecessary `elif` after `break` statement
- Packs/CheckpointFirewall/Scripts/CheckpointFWBackupStatus/CheckpointFWBackupStatus.py:30:21: RET508 Unnecessary `else` after `break` statement
+ Packs/CheckpointFirewall/Scripts/CheckpointFWBackupStatus/CheckpointFWBackupStatus.py:30:21: RET508 [*] Unnecessary `else` after `break` statement
- Packs/CheckpointFirewall/Scripts/CheckpointFWCreateBackup/CheckpointFWCreateBackup.py:30:17: RET508 Unnecessary `else` after `break` statement
+ Packs/CheckpointFirewall/Scripts/CheckpointFWCreateBackup/CheckpointFWCreateBackup.py:30:17: RET508 [*] Unnecessary `else` after `break` statement
- Packs/DeveloperTools/Scripts/WaitAndCompleteTask/WaitAndCompleteTask.py:117:9: RET508 Unnecessary `elif` after `break` statement
+ Packs/DeveloperTools/Scripts/WaitAndCompleteTask/WaitAndCompleteTask.py:117:9: RET508 [*] Unnecessary `elif` after `break` statement
... 28 additional changes omitted for project

zulip/zulip (+0 -0 violations, +422 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- analytics/views/activity_common.py:81:5: RET505 Unnecessary `else` after `return` statement
+ analytics/views/activity_common.py:81:5: RET505 [*] Unnecessary `else` after `return` statement
- analytics/views/activity_common.py:88:5: RET505 Unnecessary `else` after `return` statement
+ analytics/views/activity_common.py:88:5: RET505 [*] Unnecessary `else` after `return` statement
- analytics/views/stats.py:505:5: RET505 Unnecessary `elif` after `return` statement
+ analytics/views/stats.py:505:5: RET505 [*] Unnecessary `elif` after `return` statement
... 363 additional changes omitted for rule RET505
- corporate/lib/stripe.py:4293:9: RET506 Unnecessary `else` after `raise` statement
+ corporate/lib/stripe.py:4293:9: RET506 [*] Unnecessary `else` after `raise` statement
- corporate/tests/test_stripe.py:460:13: RET506 Unnecessary `else` after `raise` statement
+ corporate/tests/test_stripe.py:460:13: RET506 [*] Unnecessary `else` after `raise` statement
- corporate/views/remote_billing_page.py:104:9: RET506 Unnecessary `else` after `raise` statement
... 411 additional changes omitted for project

Changes by rule (4 rules affected)

code total + violation - violation + fix - fix
RET505 1434 0 0 1434 0
RET506 372 0 0 372 0
RET508 50 0 0 50 0
RET507 28 0 0 28 0

@diceroll123 diceroll123 marked this pull request as ready for review January 21, 2024 08:21
@diceroll123 diceroll123 changed the title [flake8-return] - add fix for RET505, RET506, RET507, RET508 [flake8-return] - Add fixes for (RET505, RET506, RET507, RET508) Jan 21, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks, that's a very useful fix.

I looked at the performance regression and it's caused by adjust_indentation calling into libCST. The regression is fine because it only applies to files with that specific violation. It's not that it introduces a slow down for projects that have no violations.

There's one edge case that the code isn't handling correctly but I don't think it's worth addressing. But feel free to give it a try if you want.

Would you mind adding a few more tests that demonstrate how the rule handles comments? We should then be good to merge this.

elif_else.start() + TextSize::from(2),
)))
} else {
let else_line_range = checker.locator().full_line_range(elif_else.start());
Copy link
Member

Choose a reason for hiding this comment

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

I don't expect this to exist in practice but assuming that the else header ends on the same line as the else keyword is not always true. For example if you have.

if True:
    return

else\
    :\
    # comment
    pass

it then keeps :\\n # comment where it should not. We could use SimpleTokenizer to detect this but I don't think it's worth doing.

However, I think it would be good to add a few more tests that demonstrate how the fix deals with comments in different positions.

if True:
    return
else:
    # comment
    pass
if True:
    return
else: # comment
    pass

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tweaked it to work with that line continuation that I didn't account for, and the code now keeps all comments after the else's : 😄

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Jan 24, 2024
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice!

@MichaReiser MichaReiser merged commit ffd13e6 into astral-sh:main Jan 25, 2024
17 checks passed
@MichaReiser MichaReiser added fixes Related to suggested fixes for violations and removed rule Implementing or modifying a lint rule labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants