wemake-python-styleguide icon indicating copy to clipboard operation
wemake-python-styleguide copied to clipboard

Forbid explicit step in slice

Open orsinium opened this issue 5 years ago • 10 comments

Rule request

Thesis

# bad
a[1:4:]
# good
a[1:4]

# bad
a[1::]
# good
a[1:]

# bad
a[1:4:1]
# good
a[1:4]

# bad
a[1::1]
# good
a[1:]

# bad
a[::1]
# good
a.copy()

Case with a[::] will be covered in #1011

Reasoning

Consistency

orsinium avatar Dec 11 '19 10:12 orsinium

I would like to prepare some PR adding visitor and violation for that, but after doing quick experiments, I concluded that for 1st and 2nd case the AST looks identical (I have checked it on python 3.7.4), so probably the only way to implement this violation is to use BaseTokenVisitor, which can be quite complicated, however if you agree, I can give it a try :)

Also I am not sure about the last example, because it will work only with list and other objects that has .copy(), but e.g. tuple is immutable so it doesn't have such method, so suggesting users to use .copy() instead of a[::1] can be incorrect and misleading, however we can still suggest to use a[:]

skarzi avatar Jan 26 '20 20:01 skarzi

I concluded that for 1st and 2nd case the AST looks identical

Let's for the beginning cover remaining cases. It's better than nothing.

tuple is immutable so it doesn't have such method

So, why could we want to use [::] on it if it is immutable? Also, you can always use copy.copy function for custom collections (than does nothing for tuples as well, btw).

however we can still suggest to use a[:]

As you said, we can't say the difference between a[:] and a[::]. So we can't suggest using one instead of others.

orsinium avatar Jan 27 '20 12:01 orsinium

So, why could we want to use [::] on it if it is immutable? Also, you can always use copy.copy function for custom collections (than does nothing for tuples as well, btw).

True, it doesn't make much sense when we are talking about immutable objects, I just wanted to give some example that .copy() is not a good solution, however I agree that copy.copy() is.

As you said, we can't say the difference between a[:] and a[::]. So we can't suggest using one instead of others.

If we decide to write token visitor instead of AST, it will be possible to distinguish cases like 1st and 2nd ones, but I need to do some research and experiments to fully confirm that.

skarzi avatar Jan 27 '20 13:01 skarzi

In case we cannot work with [::] and [:] even with tokens, then we can wait for 0.15 with libcst support.

sobolevn avatar Jan 27 '20 13:01 sobolevn

I have checked if it's possible to implement this violation with libast and it seems to be doable, so I will try to implement it in following week

skarzi avatar Feb 29 '20 13:02 skarzi

When do you plan to merge https://github.com/wemake-services/wemake-python-styleguide/pull/1147 to master? I am asking, because this PR introduces few features I'd like to use in my PR.

skarzi avatar Feb 29 '20 14:02 skarzi

@skarzi after 0.14 is released. I hope to finish it in two weeks.

sobolevn avatar Feb 29 '20 20:02 sobolevn