Repository navigation
fix Ruff rule B007 violation in astropy/coordinates - #17844
kangqiwang wants to merge 6 commits into
Conversation
|
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
left a comment
eerovaher
left a comment
There was a problem hiding this comment.
All pull requests contributing to #14818 must edit .ruff.toml like the opening post of the issue describes. This is required to ensure that the pull request indeed addresses all violations of a rule in a sub-package, but also prevents new violations in future code contributions.
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Another one of those ruff rules that are matters of taste -- certainly don't see much advantage of this, but it also doesn't matter much, I guess, although it adds yet another hard-to-understand hoop for new contributors.
Anyway, please revert the change commented on in-line (only replace unused names by underscores, don't make other changes).
| for subfmt, dtype, convert, _ in subfmts: | ||
| convert_result = None | ||
| for _, dtype, convert, _ in subfmts: | ||
| convert_result = convert |
There was a problem hiding this comment.
This change makes no sense - there is no need to introduce convert_result.
There was a problem hiding this comment.
when i change the code, I was confusing as well, but you can find similar issues has been discuss in B007 guidance when variable used outside the loop only
they are talking different way
1.
convert_result = None
for _, dtype, convert, _ in subfmts:
convert_result = convert
pass
return convert_result
for _, dtype, convert, _convert in subfmts:
pass
return _convert
There was a problem hiding this comment.
Ours is like #2, but we also have an else: clause that ensures we can never have no result (if, e.g., subfmts is empty). So, really the code as is is fine, and no change beyond replacing the unused variables by _ should be needed. Any other warning is a bug if ruff.
…\tests and utils and ruff file
you can find update |
| convert_result = None | ||
| for _, dtype, convert, _ in subfmts: | ||
| convert_result = convert | ||
| for _, dtype, _convert, _ in subfmts: |
There was a problem hiding this comment.
This is nicer, but why is there a need for prefixing with underscore (replacing convert with _convert)?
There was a problem hiding this comment.
it will raises when the loop variable is not used inside the loop. but it allows the variable to be marked as private, then not used in the loop, but still used outside the loop.
I personally feel B007 is trying to do too much
There was a problem hiding this comment.
Hmm, I see. That sort-of makes sense (though really only sort of... really the code could look ahead and see if the variable is used later).
A work-around that keeps the code intact would then just move the if statement inline,
for _, dtype, convert, _ in subfmts:
if np.issubdtype(val_dtype, dtype):
if convert is not None:
try:
val1, val2 = convert(val1, val2)
except Exception:
raise TypeError(
f"for {self.name} class, input should be (long) doubles, string, "
"or Decimal, and second values are only allowed for "
"(long) doubles."
)
break
else:
raise ValueError("input type not among selected sub-formats.")
Not very elegant but maybe better?
EDIT: updated above, since initially I forgot the if np.issubdtype clause.
There was a problem hiding this comment.
Might even make the control flow clearer, but otherwise I don't see much sense in restructuring this much code just because the linter can't figure out what is happening 4 lines down the code.
There was a problem hiding this comment.
@dhomeier - yes, I agree, and in a way the control flow is clearer as it is: first the loop matches the type and collects any associated convert function, and then the function is used.
|
Because this pull request is updating many sub-packages, it will require reviews from many maintainers. Having had a brief look at the code changes I can see that the reviews will not be trivial. If you want to address B007 in |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
Description
This PR ensures compliance of
astropy/coordinatesto Ruff rule unused-loop-control-variable (B007)Fixes partially #14818