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

Do not complain about -1 overusage #2364

Closed
wants to merge 9 commits into from
Closed

Do not complain about -1 overusage #2364

wants to merge 9 commits into from

Conversation

dreamflasher
Copy link
Contributor

@dreamflasher dreamflasher commented Feb 28, 2022 •
edited

Negative constants are already handled by "Found magic number".

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

Closes #2362

@dreamflasher
Do not handle negative constants here
a7ba60a 
Negative constants are already handled by "Found magic number".
sobolevn
sobolevn reviewed Feb 28, 2022
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.

Thank you!

wemake_python_styleguide/logic/complexity/overuses.py
@@ -101,13 +101,9 @@ def is_unary_minus(node: ast.AST) -> bool:

We use this predicate to allow values
like ``-some_value`` to be overused.

Although negative constants like ``-1``
Copy link
Member

Choose a reason for hiding this comment

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

-1 is fine. But, things like -5 and -37 should be reported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But they are already reported by "Found magic number" – should they be really reported again here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, complexity rules are different from "Found magic number"

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 am looking at this:

wemake-python-styleguide/wemake_python_styleguide/constants.py

Line 395 in 2377314

MAGIC_NUMBERS_WHITELIST: Final = frozenset((

Should all negatives of those be excluded or only -1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sobolevn what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Only -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sobolevn done :)

@codecov
Copy link

codecov bot commented Mar 3, 2022 •
edited

Codecov Report

Merging #2364 (0e175ae) into master (2377314) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 0e175ae differs from pull request most recent head 6d6340c. Consider uploading reports for the commit 6d6340c to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2364   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          120       120           
  Lines         6394      6392    -2     
  Branches      1442      1441    -1     
=========================================
- Hits          6394      6392    -2
Impacted Files Coverage Δ
...ake_python_styleguide/logic/complexity/overuses.py 100.00% <100.00%> (ø)

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 2377314...6d6340c. Read the comment docs.

@dreamflasher dreamflasher marked this pull request as ready for review March 3, 2022 16:10
@dreamflasher dreamflasher changed the title Do not handle negative constants in overuse code Do not complain about -1 overusage Mar 3, 2022
sobolevn
sobolevn requested changes Mar 3, 2022
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.

Thank you! Please, add a test case for this.

@sobolevn sobolevn mentioned this pull request Sep 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sobolevn sobolevn sobolevn requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WPS204: Overused expression should not count -1
2 participants
@dreamflasher @sobolevn