fix ruff rule B007 violation in astropy/io by kangqiwang · Pull Request #17880 · astropy/astropy

kangqiwang · 2025-03-12T16:36:13Z

Description

This pull request is to address to split pull request #17844 , and it ensure compliance of astropy/coordinates 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 · 2025-03-12T16:36:41Z

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.

neutrinoceros · 2025-03-13T09:07:02Z

Hi, thanks for your work. I see you've been splitting this work into smaller chunks, and I assume you mean to divide it on a per-subpackage basis, however note that astropy.io is special: it's not considered a unique subpackage, but rather, its children namespaces are, so there's still a good chance that your work ends up waiting for reviews for a while, and it'd be preferable to split one level deeper. Sorry for the inconvenience !

nstarman

Thanks for the PR @kangqiwang!

nstarman · 2025-03-13T16:30:22Z


-                for i, key in (
+                for i, _ in (
                    (i, x)


This can be simplified further by not packing x into the tuple.

nstarman · 2025-03-13T17:43:30Z

+            for spacer_line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line):
+                lines.append(spacer_line.copy())


I think this can be simplified further to an extend()

Suggested change

for spacer_line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line):

lines.append(spacer_line.copy())

lines.extend([line.copy() for line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line)])

Also, why adding the .copy() here? zip did not produce a copy...

nstarman · 2025-03-13T17:49:11Z

            res = []
-            for i in range(number):
+            for _ in range(number):
                res.append(pickle.load(f))


res = [pickle.load(f) for _ in range(number)]

nstarman · 2025-03-13T17:50:14Z

        cy.representation_component_names
    )
-    for name in c.representation_component_names:
+    for _ in c.representation_component_names:


There must be a mistake in the the test. The next line should use name.

I think that should be for attr in c.representation_component_names:.

Co-authored-by: Nathaniel Starkman <[email protected]>

taldcroft · 2025-04-11T10:40:41Z

        else:
            for i, line in enumerate(self.process_lines(lines)):
                if i == start_line:
+                    self.names = next(self.splitter([line]))


Please revert, this is a code logic change and out of scope for a ruff PR.

taldcroft · 2025-04-11T10:40:51Z

            else:  # No header line matching
                raise ValueError("No header line found in table")

-            self.names = next(self.splitter([line]))


Please revert, this is a code logic change and out of scope for a ruff PR.

⚖️ depends on one's perspective. How often do we actually look over these lines of code with a critical eye? In plenty of other ruff PRs we've embraced the opportunity to improve a line of code beyond the minimum level required to satisfy ruff.

But if you want to make an Issue and do a follow up PR now that this has been spotted, that's the more procedurally correct thing.
OTOH, the logic elsewhere has often been "great! Someone wants to contribute and is using the ruff PR to also improve code = win/win".
🎣 with lint rules 🏋️‍♀️ with more substantial code improvements. 🤣

taldcroft · 2025-04-11T10:47:27Z

-            for i, spacer_line in zip(
-                range(self.start_line), itertools.cycle(self.write_spacer_lines)
-            ):
-                lines.append(spacer_line)
+            for spacer_line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line):
+                lines.append(spacer_line.copy())


Please avoid any code logic change in this ruff PR. Your changes should mostly be limited to either changing a loop variable to _, or in some cases eliminating an unnecessary enumerate.

taldcroft · 2025-04-11T10:51:08Z

        for col in cols:
-            for key, converters in self.converters.items():
+            converters = [convert_numpy(col.dtype)] if col.dtype is not None else self.default_converters
+            for key, conv in self.converters.items():
                if fnmatch.fnmatch(col.name, key):
+                    converters=conv
                    break
-            else:
-                if col.dtype is not None:
-                    converters = [convert_numpy(col.dtype)]
-                else:
-                    converters = self.default_converters



Not sure about the intent here, but please revert all these changes.

taldcroft · 2025-04-11T10:51:24Z

-                break
-        else:  # No header line matching
-            raise InconsistentTableError("No header line found in table")
-        return line
+                return line
+        raise InconsistentTableError("No header line found in table")


Revert this change.

taldcroft · 2025-04-11T10:52:02Z

                new_names.append(name[0])
-                for i in range(1, int(name[1])):
+                for _ in range(int(name[1])-1):
                    # Add dummy columns


Revert to the original range(1, int(name[1]))).

taldcroft · 2025-04-11T10:53:25Z

+        for col in self.columns:
+            idx += 1


Here the preferred change would be to revert to the original enumerate but remove the unnecessary idx = 0 statement.

taldcroft · 2025-04-11T10:54:21Z

-                ks = range(*key.indices(axis))
+                data = [self[keys[:idx] + (k,) + keys[idx + 1 :]] for k in range(*key.indices(axis))]
                break

            if isiterable(key):
                # Handle both integer and boolean arrays.
-                ks = np.arange(axis, dtype=int)[key]
+                data = [self[keys[:idx] + (k,) + keys[idx + 1 :]] for k in np.arange(axis, dtype=int)[key]]
                break
            # This should always break at some point if _getdata is called.

-        data = [self[keys[:idx] + (k,) + keys[idx + 1 :]] for k in ks]
-


Revert these changes, they are substantively changing the code and out of scope for this PR.

taldcroft · 2025-04-11T10:56:18Z

        cy.representation_component_names
    )
-    for name in c.representation_component_names:
+    for _ in c.representation_component_names:


I think that should be for attr in c.representation_component_names:.

github-actions · 2025-08-15T05:45:35Z

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.

github-actions · 2025-09-15T05:42:54Z

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.

fix ruff rule B007 violation in astropy/io

6773991

kangqiwang requested review from WilliamJamieson, dhomeier, matteobachetti, nstarman, saimn and taldcroft as code owners March 12, 2025 16:36

github-actions Bot added testing io.votable io.fits io.ascii io.misc unified-io dev-automation labels Mar 12, 2025

eerovaher mentioned this pull request Mar 12, 2025

Ensure compliance with more Ruff rules #14818

Closed

3 of 6 tasks

pllim added this to the v7.1.0 milestone Mar 12, 2025

pllim added the no-changelog-entry-needed label Mar 12, 2025

nstarman reviewed Mar 13, 2025

View reviewed changes

Update astropy/io/ascii/html.py

30b5879

Co-authored-by: Nathaniel Starkman <[email protected]>

taldcroft requested changes Apr 11, 2025

View reviewed changes

astrofrog modified the milestones: v7.1.0, v7.2.0 Apr 28, 2025

github-actions Bot added the Close? Tell stale bot that this issue/PR is stale label Aug 15, 2025

github-actions Bot added the closed-by-bot Closed by stale bot label Sep 15, 2025

github-actions Bot closed this Sep 15, 2025

		for spacer_line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line):
		lines.append(spacer_line.copy())

	for spacer_line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line):
	lines.append(spacer_line.copy())
	lines.extend([line.copy() for line in itertools.islice(itertools.cycle(self.write_spacer_lines), self.start_line)])

Conversation

kangqiwang commented Mar 12, 2025

Description

Uh oh!

github-actions Bot commented Mar 12, 2025

Uh oh!

neutrinoceros commented Mar 13, 2025

Uh oh!

nstarman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nstarman Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 15, 2025

Uh oh!

github-actions Bot commented Sep 15, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nstarman Apr 11, 2025 •

edited

Loading