Skip to content

fix ruff rule B007 violation in astropy/wcs - #17866

Merged
pllim merged 5 commits into
astropy:mainfrom
kangqiwang:ruff-b007/wcs
Mar 24, 2025
Merged

fix ruff rule B007 violation in astropy/wcs#17866
pllim merged 5 commits into
astropy:mainfrom
kangqiwang:ruff-b007/wcs

Conversation

@kangqiwang

@kangqiwang kangqiwang commented Mar 7, 2025 •
edited by pllim
Loading

Copy link
Copy Markdown
Contributor

Description

This pull request is to address to split pull request #17844 , and it ensure compliance of astropy/wcs to Ruff rule unused-loop-control-variable (B007)

Fixes partially #14818

  • By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

@github-actions

github-actions Bot commented Mar 7, 2025

Copy link
Copy Markdown
Contributor

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.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
  • Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
  • Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
  • At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

@pllim pllim added this to the v7.1.0 milestone Mar 7, 2025
@pllim
pllim requested a review from eerovaher March 7, 2025 22:26
nstarman
nstarman requested changes Mar 9, 2025
View reviewed changes

@nstarman nstarman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @kangqiwang for the targeted PR! Looking at the code, there's an even larger simplification that can be done using a list comprehension.

Comment thread astropy/wcs/wcs.py Outdated
Comment on lines 2945 to 2948
missing_keys = []
for kw, val in full_header.items():
for kw in full_header.keys():
if kw not in header:
missing_keys.append(kw)

@nstarman nstarman Mar 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing_keys = [k for k in full_header.keys() if k not in header]

@eerovaher eerovaher Mar 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be even shorter:

missing_keys = [k for k in full_header if k not in header]

@nstarman nstarman Mar 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't check if full_header is a dict. .items() (and .keys()) doesn't guarantee it has dict-like iteration over keys, though it's definitely bad API if it doesn't iterate like that.

@kangqiwang kangqiwang Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

full_header coming from to_header() function, which return astropy.io.fits.Header

@nstarman nstarman Mar 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does that __iter__ ?

@kangqiwang kangqiwang Mar 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delayed response. I just read through the source code and documentation. I tried to find relevant test cases in test_header and attempted to verify my understanding using a for loop on header, but I couldn't find any direct tests. However, from the source code, it looks like header has both dict-like and list-like behavior for FITS headers.

here's init method:

def __init__(self, cards=[], copy=False):
        self.clear()

        if isinstance(cards, Header):
            if copy:
                cards = cards.copy()
            cards = cards.cards
        elif isinstance(cards, dict):
            cards = cards.items()

        for card in cards:
            self.append(card, end=True)

        self._modified = False

And here's iter function

    def __iter__(self):
        for card in self._cards:
            yield card.keyword

Let me know if this makes sense to you!

@nstarman nstarman Mar 17, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The __iter__ is then the same as for k in keys()?

eerovaher
eerovaher reviewed Mar 10, 2025
View reviewed changes
Comment thread .ruff.toml Resolved
@eerovaher eerovaher mentioned this pull request Mar 12, 2025
Closed
3 of 6 tasks
@neutrinoceros

neutrinoceros commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

@kangqiwang can you rebase this branch and resolve merge conflicts ?

@kangqiwang
kangqiwang force-pushed the ruff-b007/wcs branch 2 times, most recently from 7c2e0c8 to a3633bb Compare March 18, 2025 14:51
neutrinoceros
neutrinoceros reviewed Mar 18, 2025
View reviewed changes
Comment thread .ruff.toml Resolved
@kangqiwang
update ruff.toml to alphabetically sorted
b671c86 
slove merge issues

update ruff.toml

update ruff.toml

update ruff.toml
@kangqiwang

kangqiwang commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

@kangqiwang can you rebase this branch and resolve merge conflicts ?

done, let me know u find more issues

nstarman
nstarman approved these changes Mar 24, 2025
View reviewed changes

@nstarman nstarman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @kangqiwang !

@pllim
pllim merged commit 3bf68ee into astropy:main Mar 24, 2025
kangqiwang added a commit to kangqiwang/astropy that referenced this pull request Mar 24, 2025
@kangqiwang
Merge pull request astropy#17866 from kangqiwang/ruff-b007/wcs
7503ea9 
fix ruff rule B007 violation in astropy/wcs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nstarman nstarman nstarman approved these changes
@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson WilliamJamieson is a code owner
@mcara mcara Awaiting requested review from mcara mcara is a code owner
@eerovaher eerovaher Awaiting requested review from eerovaher
@neutrinoceros neutrinoceros Awaiting requested review from neutrinoceros neutrinoceros is a code owner

Assignees

No one assigned

Labels

dev-automation no-changelog-entry-needed testing wcs

Projects

None yet

Milestone

v7.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@kangqiwang @neutrinoceros @nstarman @eerovaher @pllim