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.

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

1731 Forbid returning too long tuples #1939

Merged
merged 2 commits into from
Dec 13, 2021
Merged

1731 Forbid returning too long tuples #1939

merged 2 commits into from
Dec 13, 2021

Conversation

uhbif19
Copy link
Contributor

@uhbif19 uhbif19 commented Mar 10, 2021 •
edited
Loading

I have made things!

This PR extends TooLongYieldTupleViolation realization this way:

  • Violation is new
  • Tests and visitor was extended to cover this violation
  • Constant is new (MAX_LEN_RETURN_TUPLE). I am not sure how do they work, but doc says they are a public API. So I decided I should not rename constant to cover both cases, to not to break API.

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

@codecov
Copy link

codecov bot commented Mar 10, 2021 •
edited
Loading

Codecov Report

Merging #1939 (fc029b0) into master (9faaef0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head fc029b0 differs from pull request most recent head 63fa7be. Consider uploading reports for the commit 63fa7be to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            master     #1939    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files          120       118     -2     
  Lines         6394      6210   -184     
  Branches      1424      1382    -42     
==========================================
- Hits          6394      6210   -184
Impacted Files Coverage Δ
...ake_python_styleguide/presets/topics/complexity.py 100.00% <ø> (ø)
wemake_python_styleguide/constants.py 100.00% <100.00%> (ø)
wemake_python_styleguide/violations/complexity.py 100.00% <100.00%> (ø)
...ython_styleguide/visitors/ast/complexity/counts.py 100.00% <100.00%> (ø)
wemake_python_styleguide/logic/nodes.py 100.00% <0.00%> (ø)
wemake_python_styleguide/violations/oop.py 100.00% <0.00%> (ø)
wemake_python_styleguide/logic/tree/classes.py 100.00% <0.00%> (ø)
wemake_python_styleguide/options/validation.py 100.00% <0.00%> (ø)
wemake_python_styleguide/presets/types/tree.py 100.00% <0.00%> (ø)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9faaef0...63fa7be. Read the comment docs.

sobolevn
sobolevn reviewed Mar 11, 2021
View reviewed changes
wemake_python_styleguide/constants.py Outdated
@@ -369,6 +369,9 @@
#: Maximum length of ``yield`` ``tuple`` expressions.
MAX_LEN_YIELD_TUPLE: Final = 5

#: Maximum length of ``return`` ``tuple`` expressions.
MAX_LEN_RETURN_TUPLE: Final = 5
Copy link
Member

Choose a reason for hiding this comment

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

I think that we should unify this setting into a single one. How about MAX_LEN_TUPLE_OUTPUT?

wemake_python_styleguide/violations/complexity.py Outdated


@final
class TooLongReturnTupleViolation(ASTViolation):
Copy link
Member

Choose a reason for hiding this comment

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

Let's rename the existing Yield violation. I think that there's no need to create a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

Are there any docs in which cases changing violations or some parts of public API is ok?

Copy link
Contributor Author

@uhbif19 uhbif19 Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

All done. I am not sure about naming through.

Copy link
Member

Choose a reason for hiding this comment

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

Are there any docs in which cases changing violations or some parts of public API is ok?

We don't consider our code to be a part of our API, because no one in their sane minds would use it. This is a CLI tool, so only UX changes are important.

Copy link
Contributor Author

@uhbif19 uhbif19 Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I just am confused by the comment lines in constants.py. There is a separation between constants public for "end-user" and internal which are not documented. So I do not get, what these lines are talking about if it is not about API.

https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/constants.py#L25

@uhbif19 uhbif19 requested a review from sobolevn March 11, 2021 18:07
sobolevn
sobolevn reviewed Mar 11, 2021
View reviewed changes
Copy link
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!

tests/test_visitors/test_ast/test_complexity/test_counts/test_function_output_length.py Outdated
from wemake_python_styleguide.violations.complexity import (
TooLongYieldTupleViolation,
TooLongFunctionOutputTupleViolation,
Copy link
Member

Choose a reason for hiding this comment

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

Let's name it TooLongOutputTupleViolation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Also renamed test file accordingly.

@uhbif19 uhbif19 requested a review from sobolevn March 11, 2021 20:16
sobolevn
sobolevn approved these changes Mar 12, 2021
View reviewed changes
Copy link
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.

Awesome! Thanks!

I am going to release 0.15.3 and then merge this PR into 0.16

@sobolevn sobolevn added this to the Version 0.16 milestone Mar 12, 2021
@sobolevn sobolevn merged commit a203c25 into wemake-services:master Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn approved these changes

Assignees
No one assigned
Labels
on hold
Projects
None yet
Milestone
Version 0.16
Development

Successfully merging this pull request may close these issues.

Forbid returning too long tuples
2 participants
@uhbif19 @sobolevn