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

Added rule to enfore stricter slicing operations and unit tests for it. #1069

Closed
wants to merge 3 commits into from
Closed

Added rule to enfore stricter slicing operations and unit tests for it. #1069

wants to merge 3 commits into from

Conversation

Eric4Jiang
Copy link

I have made things!

Checklist

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

Related issues

sobolevn
sobolevn requested changes Dec 11, 2019
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! Almost done!

We have to also update tests/fixtures/noqa.py and tests/test_checker/test_noqa.py to enable an integration test.

CHANGELOG.md Outdated
@@ -82,6 +82,7 @@ It features a lot of new rules from different categories.
- Forbids to use and declare `float` keys in arrays and dictionaries
- Forbids to use `a[len(a) - 1]` because it is just `a[-1]`
- Forbids too long call chains like `foo(a)(b)(c)(d)`
- Enforces stricter slice operation usage
Copy link
Member

Choose a reason for hiding this comment

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

This should go to 0.15.0 section

Copy link
Author

Choose a reason for hiding this comment

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

Could you be more specific? I don't see a section 0.15.0 in CHANGELOG.md. It only goes up to 0.13.3

Copy link
Member

Choose a reason for hiding this comment

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

That's what you have to create 🙂

Copy link
Author

Choose a reason for hiding this comment

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

Okay! Could you explain reasoning why this is going into a new section rather than 0.13.0?

Copy link
Member

Choose a reason for hiding this comment

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

Because 0.13 is already released, and we are working on 0.15

tests/test_visitors/test_ast/test_subscripts/test_undescriptive_slicer_operation.py Outdated
'3::2',
'5:7:',
':7:2',
'3::',
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this subscript also does not make any sense. Why would anyone want to write 3:: and not just 3:?

Copy link
Member

Choose a reason for hiding this comment

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

Related #1071

tests/test_visitors/test_ast/test_subscripts/test_undescriptive_slicer_operation.py Outdated
'5:7:',
':7:2',
'3::',
':7:',
Copy link
Member

Choose a reason for hiding this comment

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

The same here. Why not just :7?

wemake_python_styleguide/violations/best_practices.py Outdated
# Wrong:
items[::-1]

.. versionadded:: 0.13.0
Copy link
Member

Choose a reason for hiding this comment

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

.. versionadded:: 0.15.0

wemake_python_styleguide/violations/best_practices.py Outdated
"""

error_template = 'Found undescriptive slice operation'
code = 500
Copy link
Member

Choose a reason for hiding this comment

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

Code is not correct. This violation should be located in refactoring.py and have a correct code.

wemake_python_styleguide/visitors/ast/subscripts.py
@@ -23,9 +23,10 @@ def visit_Subscript(self, node: ast.Subscript) -> None:

Raises:
RedundantSubscriptViolation

UndescriptiveSliceOperationViolation
Copy link
Member

Choose a reason for hiding this comment

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

Missing new line after this

@Eric4Jiang
Copy link
Author

Eric4Jiang commented Dec 11, 2019 •
edited
Loading

In tests/test_checker/test_noqa.py, what should my WPS code be? In the docs, I see ranges for naming, complexity, consistency, and best_practices, but no WPS range for refactoring. Are WPS from 500-600 and 600-700 for anything specific?

@Eric4Jiang
Copy link
Author

@sobolevn Just wanted to bump this in case you didn't see my last question.

@sobolevn
Copy link
Member

sobolevn commented Dec 23, 2019 •
edited
Loading

@Eric4Jiang oh, sorry. I have missed it. The range for refactoring is 5xx. Can you please add it to the docs as well?

@sobolevn sobolevn added this to the Version 0.15 milestone Jan 19, 2020
@sobolevn
Copy link
Member

I am closing this for now, feel free to reopen if you still want to work on this. Cheers! 👍

@sobolevn sobolevn closed this Feb 17, 2020
@sobolevn sobolevn modified the milestones: Version 0.16, Version 0.15 aka New runtime Oct 20, 2020
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
Version 0.15 aka Python3.9
Development

Successfully merging this pull request may close these issues.
2 participants
@Eric4Jiang @sobolevn