Skip to content

Revert PR 17461 (Optimize getting config item values)#18144

Merged
pllim merged 2 commits into
astropy:mainfrom
pllim:revert-pr17461
May 14, 2025
Merged

Revert PR 17461 (Optimize getting config item values)#18144
pllim merged 2 commits into
astropy:mainfrom
pllim:revert-pr17461

Conversation

@pllim

@pllim pllim commented May 13, 2025

Copy link
Copy Markdown
Member

Description

This pull request is to close #18138 to unblock astropy v7.1 release. This reverts commit 49765f4.

  • 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.

@pllim pllim added this to the v7.1.0 milestone May 13, 2025
@pllim pllim requested a review from saimn May 13, 2025 16:54
@pllim pllim added Bug skip-changelog-checks Tells bot to skip changlog checks backport-v7.1.x labels May 13, 2025
@github-actions

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 commented May 13, 2025

Copy link
Copy Markdown
Member Author

pre-commit.ci autofix

@mhvk

mhvk commented May 13, 2025

Copy link
Copy Markdown
Contributor

I think it makes sense to revert to unblock 7.1, especially as the fix for the problems uncovered does not seem trivial. But I'll let @saimn look before approving.

@pllim pllim merged commit 79fb7d6 into astropy:main May 14, 2025
26 of 27 checks passed
@pllim pllim deleted the revert-pr17461 branch May 14, 2025 16:30
meeseeksmachine pushed a commit to meeseeksmachine/astropy that referenced this pull request May 14, 2025
@pllim

pllim commented May 14, 2025

Copy link
Copy Markdown
Member Author

Thanks for your understanding. I will reopen the issue that the reverted PR closed.

pllim added a commit that referenced this pull request May 14, 2025
…144-on-v7.1.x

Backport PR #18144 on branch v7.1.x (Revert PR 17461 (Optimize getting config item values))
@bsipocz

bsipocz commented May 28, 2025

Copy link
Copy Markdown
Member

FYI: this broke astroquery docs build astropy/astroquery#3326

@bsipocz

bsipocz commented May 28, 2025

Copy link
Copy Markdown
Member

More precisely, this triggered #17546 to break astroquery. What I see downstream for all the type annotation PRs is pure net negative.

@mhvk

mhvk commented May 28, 2025

Copy link
Copy Markdown
Contributor

@bsipocz - I think you are not commenting on the right PR - this one had nothing to do with typing. I mention it in part since I very much think that if typing is causing down-stream problems, those so keen to introduce it need to know about it and ensure it doesn't happen again.

@bsipocz

bsipocz commented May 28, 2025

Copy link
Copy Markdown
Member

Yes, I suppose that was a badly executed manual bisect as at first I only compared this one and the previous change, but I only checked that one in its backported version that ended up in 7.0.2.

The real breaking PR is the typing one mentioned in my previous comment, rolling back those ConfigItem generator types fixes the docs build...

@bsipocz

bsipocz commented May 28, 2025

Copy link
Copy Markdown
Member

And I cannot emphasize it enough how frustrating all this typing stuff is, along with random ruff rules, it's not the first time it causes unexpected issues downstream forcing others to clean up the mess. (This particular sphinx one is something I can only workaround but not fix downstream)

@mhvk mhvk mentioned this pull request May 28, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug config skip-changelog-checks Tells bot to skip changlog checks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

reload_config no longer properly reloads user-set values

4 participants