Skip to content

Comments

Ignore ruff error PLE0605.#15385

Merged
pllim merged 1 commit intoastropy:mainfrom
draghuram:ruff-PLE0605
Sep 29, 2023
Merged

Ignore ruff error PLE0605.#15385
pllim merged 1 commit intoastropy:mainfrom
draghuram:ruff-PLE0605

Conversation

@draghuram
Copy link
Contributor

@draghuram draghuram commented Sep 25, 2023 •
edited
Loading

Ruff is getting confused by dynamic computation of __all__ so for now, the check is ignored using "noqa" comment.

Description

This pull request is to address the ruff error PLE0605.

  • 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
Copy link
Contributor

github-actions bot commented Sep 25, 2023

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?
  • Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
  • 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 v6.0 milestone Sep 25, 2023
pllim
pllim reviewed Sep 25, 2023
View reviewed changes
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Is noqa the right fix? Maybe @nstarman knows.

nstarman
nstarman requested changes Sep 25, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Thanks @draghuram! Right approach. Some comments.

astropy/logger.py
for level in logging_levels:
globals()[level] = getattr(logging, level)
__all__ += logging_levels
__all__ += logging_levels # noqa: PLE0605
Copy link
Member

@nstarman nstarman Sep 25, 2023

Choose a reason for hiding this comment

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

I think the logging_levels variable can be created in conjunction with __all__, similar to this example where a -> __all__ and b -> logging_levels. Then this whole line can be removed.

CleanShot 2023-09-25 at 17 03 28@2x

Copy link
Contributor Author

@draghuram draghuram Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@nstarman, Do you mean as follows?

__all__ += (logging_levels := ...

I tried this and we still get ruff warning. Are you ok to keep the original change?

Copy link
Member

@nstarman nstarman Sep 26, 2023

Choose a reason for hiding this comment

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

Hm. I don't think we should be getting a ruff warning from that. As in, if we are there's a bug in ruff!

__all__ =  ["a", "b"]
__all__ += (logging_levels := ["c", "d"])

Should be identical to

__all__ =  ["a", "b"]
__all__ += ["c", "d"]

from the perspective of __all__.

Copy link
Member

@nstarman nstarman Sep 26, 2023

Choose a reason for hiding this comment

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

I opened astral-sh/ruff#7672.
But it's fine to add the noqa for now.

Copy link
Contributor Author

@draghuram draghuram Sep 28, 2023

Choose a reason for hiding this comment

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

Thanks. In that case, the PR can be merged assuming there are no other concerns? I did make the other change requested already.

astropy/modeling/projections.py Outdated


__all__ = [
__all__ = [ # noqa: PLE0605
Copy link
Member

@nstarman nstarman Sep 25, 2023

Choose a reason for hiding this comment

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

Instead of this, please split the __all__ to

__all__ = [...]
__all__ += + list(map("_".join, product(["Pix2Sky", "Sky2Pix"], chain(*_PROJ_NAME_CODE))))  # noqa: PLE0605

Copy link
Member

@nstarman nstarman Sep 25, 2023

Choose a reason for hiding this comment

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

@WilliamJamieson, do you want to keep this dynamic or actually enumerate the list for static purposes?

@draghuram
Ignore ruff error PLE0605.
3e3f9c9 
Ruff is getting confused by dynamic computation of "__all__" so for now,
the check is ignored using "noqa" comment.

Signed-off-by: Raghuram Devarakonda <[email protected]>
@nstarman nstarman mentioned this pull request Sep 26, 2023
Closed
nstarman
nstarman approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@nstarman nstarman left a comment

Choose a reason for hiding this comment

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

Thanks @draghuram !

@pllim pllim merged commit e135a78 into astropy:main Sep 29, 2023
@pllim
Copy link
Member

pllim commented Sep 29, 2023

Thanks, all!

@eerovaher eerovaher mentioned this pull request Oct 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments

@nstarman nstarman nstarman approved these changes

@WilliamJamieson WilliamJamieson Awaiting requested review from WilliamJamieson WilliamJamieson is a code owner

Assignees

No one assigned

Labels

logging modeling no-changelog-entry-needed utils.masked

Projects

None yet

Milestone

v6.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@draghuram @pllim @nstarman