Skip to content

Conversation

DimitriPapadopoulos
Copy link
Contributor

In preparation for #24994.

@mhvk
Copy link
Contributor

mhvk commented Aug 27, 2024

This also has linting errors! (So I didn't review yet)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Aug 27, 2024

The linting errors are not caused by existing code, not by my changes. I would like to add noqa: E231 in this specific case. Thoughts?

@mhvk
Copy link
Contributor

mhvk commented Aug 27, 2024

Probably best to just fix them - best not to merge PRs which do not pass the tests...

@DimitriPapadopoulos
Copy link
Contributor Author

Should be fixed by running ruff check --select E231 on this specific file (requires --preview by the way):

$ ruff check --preview --select E231 numpy/_core/tests/test_nditer.py --fix
Found 33 errors (33 fixed, 0 remaining).
$ 

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Hmm, weird that that was the problem, especially as there seem to be worse problems (at least visually).

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the C4 branch 2 times, most recently from 64a8569 to a887b51 Compare August 27, 2024 21:30
Copy link
Member

@charris charris left a comment

Choose a reason for hiding this comment

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

Couple of ugly spots, otherwise looks OK. A few tricky bits, but mostly straightforward.

C410 Unnecessary `list` literal passed to `list()`
     (remove the outer call to `list()`)
C414 Unnecessary `list` call within `sorted()`
C416 Unnecessary `list` comprehension (rewrite using `list()`)
E231 Missing whitespace after ','

Apply to a single file to please the current linter.
C417 Unnecessary `map` usage (rewrite using a generator expression)
C419 Unnecessary list comprehension
@charris charris merged commit dc5419f into numpy:main Aug 30, 2024
66 checks passed
@charris
Copy link
Member

charris commented Aug 30, 2024

Thanks @DimitriPapadopoulos .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants