Forbid consecutive slices

[sobolevn]

Rule request

Thesis

We should forbid consecutive slices.

Reasoning

Consider this code:

>>> a = [1, 2, 3, 4]
>>> a[1:][:2]
[2, 3]

What it does?

1. a[1:] is [2, 3, 4]
2. [2, 3, 4][:2] is [2, 3]

It is much more readable and effective to use

>>> a[1:3]
[2, 3]

Related https://twitter.com/m_ou_se/status/1403426221367627777

[Edald123]

Hi! I'm new and I would like to work on this issue. May I ask for more info?

[sobolevn]

@Edald123 yes, sure! Thanks!

Please, take a look at https://github.com/wemake-services/wemake-python-styleguide/blob/master/CONTRIBUTING.md
Feel free to ask any questions.

[Edald123]

Thank you! I'm on it

[Edald123]

Hi, I have already checked the resources from the contributing file. I would like to check if my approach would be correct or not. I understood that I would have to create a new visitor and violation, not necessarily a new flake8 plugin. From there I would choose an ast based visitor using the already created one: BaseNodeVisitor. Then I would write only a violation and check logic.

Would this be a good approach taking into account the type of rule of the issue? Thaks!

[sobolevn]

I would have to create a new visitor and violation

Correct.

not necessarily a new flake8 plugin

Not a plugin, correct!

From there I would choose an ast based visitor using the already created one: BaseNodeVisitor

Yes 👍

Then I would write only a violation and check logic

Correct! I will be glad to review your code and help with any questions you have.

[Edald123]

Thank you!, I will be sharing my progress then.

[Edald123]

Hi!
Just as an update. I'm still working on the issue. I got stuck at creating the logic for the violation. I'll be trying to solve this to give you and update later.

[sobolevn]

Sure! Thanks!

@Edald123 do you need my help?

[Edald123]

Hi!
I actually worked on a solution with Gibran.

So far we have implemented a solution in wemake_python_styleguide/visitors/ast/subscripts.py.
In the class SubscriptVisitor we added:

def _check_consecutive(self, node: ast.Subscript) -> None:
        """Check if subscript node has a slice and a subscript"""
        if isinstance(node.slice, ast.Slice) and isinstance(node.value, ast.Subscript):
            self.add_violation(consistency.ForbidConsecutiveSlicesViolation(node))

Our implementation is passing some tests but others not.
The tests we created are for:

no_consecutive = """
a = [1, 2, 3, 4]
b = a[1:]
"""

no_consecutive_visit = """
a = [1, 2, 3, 4]
b = a[1:][2]
"""

no_slicing = """
a = [1, 2, 3, 4]
b = a[1]
"""

no_slicing_double = """
a = [1, [5, 6, 7], 3, 4]
b = a[1][3]
"""

no_slicing_triple = """
a = [1, [5, 6, 7, [8, 9, 10]], 3, 4]
b = a[1][3][1]
"""

consecutive_double = """
a = [1, 2, 3, 4]
b = a[1:][2:]
"""

consecutive_triple = """
a = [1, 2, 3, 4, 5, 6]
b = a[1:][2:][:2]
"""

consecutive_plus = """
a = [1, [5, 6, 7, 8, 9, 10], 3, 4]
b = a[1][2:][:4]
"""

no_consecutive_for_slices = """
a = [1, [5, 6, 7, [8, [1, 2, 3, 4], 10]], 3, 4]
for i in a[1][3][1]:
    print(i)
"""

consecutive_for = """
a = [1, [5, 6, 7, [8, [1, 2, 3, 4], 10]], 3, 4]
for i in a[1][:4][1:]:
    print(i)
"""

The tests that are not supposed to raise errors are:
no_consecutive, no_consecutive_visit, no_slicing, no_slicing_double, no_slicing_triple, no_consecutive_for_slices.
These are working fine.

The tests that are supposed to raise an error (violation) are:
consecutive_double, consecutive_triple, consecutive_plus, consecutive_for.
Of these tests only consecutive_double is passing since we are expecting the raise of 1 violation and it is raising 1
The other three are failing: consecutive_triple, consecutive_for, and consecutive_plus.

The problem is that these 3 tests are raising 2 errors instead of 1 so they fail: AssertionError: assert 1 == 2
This failure is occurring in tests/test_visitors/conftest.py, in the assert_errors function, in the line assert len(errors) == len(real_errors)
Therefore, it seems like the code is working for forbidding 2 consecutive slices, but when there are more it isn't.
We are working on solving this.

[sobolevn]

Hey, send a work-in-progress PR! It would be super helpful! 👍

[GibranHL0]

Hey @sobolevn

Like @Edald123 commented, we worked in a pair together to solve the issue. A PR is going to be uploaded in a few seconds.

[Edald123]

Hi!
@GibranHL0 will submit the PR in a minute. We will be waiting for your comments, thank you!

[Manthanjain]

Hi @sobolevn I am new to open source and wanted to get started with contributing to good first issues, but i am facing a little difficulty in how to get myself more invoved in the project. Can you suggest me what steps do i need to follow after going through contributing docs.

[sobolevn]

Hi @Manthanjain! Sure!

Read this:

• https://wemake-python-stylegui.de/en/latest/pages/api/index.html
• https://wemake-python-stylegui.de/en/latest/pages/api/index.html#creating-a-new-rule

But, looks like someone else is working on this issue, feel free to choose any other one.

[lensvol]

Is this issue still being worked on? Can I take it over if it's not?

[sobolevn]

We have an unofficial policy to reassign tasks after a ling period of inactivity.

Thanks a lot to everyone who wanted to help! I appreciate it ❤️

[GibranHL0]

Hi @sobolevn and @lensvol, I believe that this issue has a pull request already, but it is waiting to get merged #2094

[sobolevn]

Oups, sorry! I've totally missed it (because it was not linked to this issue).
This PR is merged: #2220