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.

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

FURB118, lambda x: x[:, 0] should be itemgetter((slice(None), 0)), instead of itemgetter((:, 0)) #13508

Closed
nasyxx opened this issue Sep 25, 2024 · 4 comments · Fixed by #13518
Assignees
Labels
bug Something isn't working rule Implementing or modifying a lint rule

Comments

@nasyxx
Copy link

nasyxx commented Sep 25, 2024

In refurb, FURB118, lambda x: x[:, 0] should be itemgetter((slice(None), 0)), instead of itemgetter((:, 0))

# xx.py
import numpy as np
xs = np.array([[1, 2, 3]])
xm = map(lambda x: x[:, 0], xs)
xx.py:
  3:10 FURB118 [*] Use `operator.itemgetter((:, 0))` instead of defining a lambda

I guess (:, 0) is not a valid tuple.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule bug Something isn't working labels Sep 25, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Sep 25, 2024

The python slice index syntax always trips me up and in this case it's unclear to me what the result of the map operation is.

@nasyxx could you clarify. Do you expect the above to be valid code? I'm asking to better understand if this is a bug in the fix or that the rule shouldn't flag the above code. CC: @AlexWaygood

@nasyxx
Copy link
Author

nasyxx commented Sep 25, 2024

@nasyxx could you clarify. Do you expect the above to be valid code?

Hi, the above is valid, while it will raise IndexError.

My actual ndarray is quite complex, you can change xs to xs = np.ones((5, 4, 3, 2, 1)). Then when running list(xm) will give you result.

@AlexWaygood
Copy link
Member

There's definitely a serious bug here: we currently suggest invalid syntax (and, if you run with --fix, we introduce invalid syntax).

- lambda x: x[:, 0]
+ import operator
+ operator.itemgetter((:, 0))

In my opinion, itemgetter((slice(None), 0)) (which would be the correct way of rewriting this with itemgetter, as @nasyxx says) is quite ugly, so I think I'd vote for not emitting a diagnostic at all in this case. But others may feel differently; emitting a diagnostic would be consistent with the overall (quite opinionated) philosophy of FURB118.

@zanieb zanieb self-assigned this Sep 25, 2024
zanieb added a commit that referenced this issue Sep 26, 2024
There was already handling for the singleton `x[:]` case but not the tuple case.\nCloses #13508
zanieb added a commit that referenced this issue Sep 26, 2024
There was already handling for the singleton `x[:]` case but not the tuple case.\nCloses #13508
@zanieb
Copy link
Member

zanieb commented Sep 26, 2024

We already handle converting x[:] to slice(None) so I opted to continue emitting the diagnostic.

zanieb added a commit that referenced this issue Sep 26, 2024
There was already handling for the singleton `x[:]` case but not the tuple case.\nCloses #13508
@zanieb zanieb closed this as completed in 58a8e9c Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants