Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add --disable-plugin-autoload #13253

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Feb 25, 2025

fixes #8969

I'm pretty sure the implementation is solid, but I have a bunch of question marks in comments that should be resolved before merging. I can probably resolve them on my own by digging deep enough into the innards, but wouldn't mind explanations if somebody is more familiar with that territory.

I'm not entirely sure that test_installed_plugin_rewrite should be parametrized to hell, as opposed to creating a dedicated test.

I also found a random unrelated typo in test_installed_plugin_rewrite with check_first -> check_first2, so fixed that + improved the fnmatch_lines.

  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.
  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.

@jakkdl jakkdl requested a review from Zac-HD February 25, 2025 16:45
@Zac-HD
Copy link
Member

Zac-HD commented Feb 26, 2025

I don't have answers for those questions, sorry, but I'm happy to say that the PR is looking good overall, and I'm looking forward to merging it once they're resolved and CI is green 🙂

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Feb 27, 2025
@jakkdl
Copy link
Member Author

jakkdl commented Feb 27, 2025

I can't repro the pypy fail locally, but I'm not sure if spec matters, so ??

I've updated comments to something that almost makes sense to merge with, but ... it's a mess.

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd appreciate a second opinion before merging. Maybe @nicoddemus?

else:
assert PseudoPlugin.attrs_used == []
# it should load if it's enabled, or we haven't disabled autoloading
assert has_loaded == bool(enable_plugin_method) or not disable_plugin_method
Copy link
Member

Choose a reason for hiding this comment

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

using parens here would be good to make the precedence more obvious

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable plugin autoload via configuration file
2 participants