Skip to content

Issue 3458#3487

Merged
sobolevn merged 3 commits intowemake-services:masterfrom
NikitaSemenovAiforia:issue-3458
Jul 12, 2025
Merged

Issue 3458#3487
sobolevn merged 3 commits intowemake-services:masterfrom
NikitaSemenovAiforia:issue-3458

Conversation

@NikitaSemenovAiforia
Copy link
Copy Markdown
Contributor

@NikitaSemenovAiforia NikitaSemenovAiforia commented Jul 12, 2025

I have made things!

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Comment

The previous solution used a number of lines to determine finally block complexity, which works purely with a function with multiple arguments or a function with a long name. New logic uses the same logic as try body violation: at most one statement.

Docs

I also changed poetry shell command in docs cause it was change since 2.0.0 version

mrmarone added 2 commits July 12, 2025 13:16
…tivate" cause this the right way to run shell command in 2.0.0+
…ber of lines to counting the number of statements.

It now works the same way as the try block. This allows using a single function with many arguments or a function with a long name that splits across multiple lines.

Option "max-lines-in-finally" renamed to "max-finally-body-length" to follow new logic.
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 12, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (991c4c9) to head (b66e680).
⚠️ Report is 42 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #3487   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          364       363    -1     
  Lines        12040     12032    -8     
  Branches       823       822    -1     
=========================================
- Hits         12040     12032    -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment thread CHANGELOG.md Outdated
### Bugfixes

- Fixes `WPS243` to use number of statements in `finally` body instead of a number of lines, #3458
- Rename `--max-lines-in-finally` option to `--max-finally-body-length`, #3458
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a breaking change :(
Sadly, we can't do that. Please, revert.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

May I keep the old name for the option but use the new one inside code? The new name follows the same pattern as try violation. Or rename internal variable as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also had to set the default value = 2 to support backward compatibility. It is slightly looser strictness than for the try body.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed one more time =) now should be fine.


Configuration:
This rule is configurable with ``--max-lines-in-finally``.
This rule is configurable with ``--max-finally-body-length``.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can solve this problem with better docs. We can explain what "line" means here. It basically means "statement"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@NikitaSemenovAiforia NikitaSemenovAiforia force-pushed the issue-3458 branch 2 times, most recently from 98639b1 to c0995dd Compare July 12, 2025 08:11
Copy link
Copy Markdown
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thank you for the quick fix! I will release this tomorrow 🤝

@sobolevn sobolevn merged commit 2d0c910 into wemake-services:master Jul 12, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forbid "fat" finally

3 participants