Remove "indx" from codespell ignore list#17725
Conversation
eerovaher
requested review from
WilliamJamieson,
astrofrog,
nstarman and
saimn
as code owners
github-actions
bot
added
testing
io.fits
units
samp
skip-changelog-checks
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
eerovaher
force-pushed
the
codespell-2-4-1
branch
from
038e490 to
ded7054
Compare
| indx.remove(x) | ||
| tmp = [self[i] for i in indx] | ||
| return ColDefs(tmp) | ||
| return ColDefs([self[i] for i in range(len(self)) if i not in _other]) |
There was a problem hiding this comment.
As far as I can tell ColDefs is not an iterable, so it is not possible to use enumerate(self) here.
There was a problem hiding this comment.
A quick trial seems to suggest ducktyping is in good form for enumerating: just having __getitem__ seems to be enough:
class A:
def __getitem__(self, i):
if i < 4:
return "abc"[i]
raise IndexError
for i, a in enumerate(A()): print(i, a)
0 a
1 b
2 c
There was a problem hiding this comment.
The code here is not covered by tests, so I don't feel very comfortable editing it more than I already have.
|
I agree this is generally better than my own #17720, which I can close now. |
nstarman
left a comment
nstarman
left a comment
There was a problem hiding this comment.
Thanks @eerovaher.
Approving, pending approval for changes to fits stuff.
|
I just merged #17720 so this might need rebase. I apologize for any inconvenience caused. |
eerovaher
force-pushed
the
codespell-2-4-1
branch
from
ded7054 to
4f0ae51
Compare
eerovaher
changed the title
codespell to 2.4.1codespell ignore list
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Agreed that this is nicer! One comment, but fine to merge without addressing that.
| indx.remove(x) | ||
| tmp = [self[i] for i in indx] | ||
| return ColDefs(tmp) | ||
| return ColDefs([self[i] for i in range(len(self)) if i not in _other]) |
There was a problem hiding this comment.
A quick trial seems to suggest ducktyping is in good form for enumerating: just having __getitem__ seems to be enough:
class A:
def __getitem__(self, i):
if i < 4:
return "abc"[i]
raise IndexError
for i, a in enumerate(A()): print(i, a)
0 a
1 b
2 c
astropy/io/fits/hdu/groups.py
Outdated
|
|
||
| # if more than one group parameter have the same name, the | ||
| # value must be a list (or tuple) containing arrays | ||
| elif not isinstance(value, (list, tuple)) or len(index) != len(value): |
There was a problem hiding this comment.
Here I'd switch the order, i.e., keep the original if but just make it an elif (no not, or->and) and write an else that raises at the end.
There was a problem hiding this comment.
I had to rebase anyways to solve a merge conflict.
In many places variables named `indx` were renamed to `index`, but in a few places it was possible to rewrite the code to avoid the variables entirely.
eerovaher
force-pushed
the
codespell-2-4-1
branch
from
4f0ae51 to
1a01ffc
Compare
|
Yes, fine to leave the iteration alone! Feel free to merge. |
|
3 approvals, and the branch already required 2 rebases in as many days. I'll just push the button now. Thanks ! |
5 participants
Description
The upgraded
codespelldoesn't like the variable nameindx. In #17720 those variables are renamed toindex, butindxis quite a common name, so the question was raised in that pull request if the code churn is worth it. In many places it is simple enough to rewrite the code so that theindx/indexvariable is not needed at all. I thought that if we are editing the code becausecodespellanyways then we might as well edit it properly, so I am opening this as an alternative for #17720.EDIT: Close #17720
I also found a line of code in
io.fitsthat I suspect has a bug in it. It is possible the labels might have to change the labels if maintainer confirms. See the inline comment for details.UPDATE
#17720 has been merged, so now this pull request only removes "indx" from the
codespellignore list.