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.

Sign up for GitHub

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

Jump to bottom

refactor: Bot.load_extensions rework #796

Open
wants to merge 36 commits into
base: master
Choose a base branch
from
Open

refactor: Bot.load_extensions rework #796

wants to merge 36 commits into from

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Oct 11, 2022 •
edited

Summary

This PR refactors Bot.load_extensions, improving several aspects and hopefully making common patterns easier to use.
Module discovery now works similar to pkgutil.walk_packages.

Initially I removed utils.search_directory entirely, but then opted to keep it for now and only deprecate it. While the transition to Bot.find_extensions (or even just Bot.load_extensions) should be trivial, having a deprecation period still won't hurt.

It might look like a lot, but most of it is documentation and tests :p

Docs can be found here https://disnake--796.org.readthedocs.build/en/796/ext/commands/extensions.html#ext-commands-extensions-load.

related: #1132

TODO

  • make sure to clear up confusions between cwd and sys.path relative to main module

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@shiftinv
refactor!: rewrite Bot.load_extensions and corresponding utils method
67bea2f
@shiftinv
feat: yield loaded extension names
60e7622
@shiftinv
fix: catch ValueError raised by resolve_name on 3.8
0c9182b
@shiftinv
fix: change exception types
8b8db8f
@shiftinv
feat: add return_exceptions parameter
5e2ada4
@shiftinv
feat: discover all ext names first
42ccd93
@shiftinv
chore: rename walk_extensions to walk_modules
82300de
@shiftinv
docs: improve walk_modules docs
9c4e784
@shiftinv
fix: update load_extensions return type
f3b24db 
not really necessary since Iterator is covariant, but might as well do it anyway
@shiftinv
fix: make walk_modules:ignore less complex
33819be
@shiftinv
feat: avoid duplicate __path__ entries
2b7d78a
@shiftinv
fix(test): update walk_modules tests
a09045e
@shiftinv
fix: un-deprecate load_extensions(<path>)
ea069ae
@shiftinv
docs: improve exception documentation
2ff26be
@shiftinv
fix: use proper error type
be6122e
@shiftinv
feat: update test_bot.__main__
3610789
@shiftinv
fix: update module not found exception
460cdf3
@shiftinv
feat: add load_callback parameter, remove iterator
3084698
@shiftinv
refactor: move to separate find_extensions method for easier customiz…
7690556 
…ation
@shiftinv
chore: use list instead of sequence
08033a7
@shiftinv
docs: mention find_extensions for customization
2f28f20
@shiftinv
chore(test): move directory utils to separate file
b402320
@shiftinv
test: add some simple tests
e83249c
@shiftinv
fix: python 3.8 moment
3be2be5
@shiftinv
docs: add changelog entries
413a5a3
@shiftinv
revert: remove return_exceptions, unnecessary complexity
c6a779b
@shiftinv
chore: move to separate documentation
e159c7e
@shiftinv
fix: reinstate utils.search_directory for now
1a3b309
@shiftinv shiftinv added t: enhancement New feature s: needs review Issue/PR is awaiting reviews labels Oct 11, 2022
@shiftinv shiftinv added the t: deprecation Deprecation of existing features label Oct 11, 2022
@shiftinv shiftinv added this to the disnake v2.7 milestone Oct 11, 2022
Enegg
Enegg reviewed Nov 15, 2022
View reviewed changes
disnake/utils.py Outdated Show resolved Hide resolved
@shiftinv shiftinv removed this from the disnake v2.8 milestone Feb 5, 2023
@Enegg
Copy link
Contributor

Enegg commented Mar 31, 2023 •
edited

Recently I've realized there are few ways of passing stuff around that doesn't involve slapping said stuff as bot attribute.
I could see a system where apart from accepting the client/bot instance in extensions' setup function, users could define extra arguments for the function of arbitrary type, which would be passed the relevant objects from the call to .load_extensions (probably as *args).
I reckon this would go into a separate PR but I am mentioning this here as it'd likely depend on this PR.

@shiftinv
Copy link
Member Author

shiftinv commented Apr 4, 2023 •
edited

Recently I've realized there are few ways of passing stuff around that doesn't involve slapping said stuff as bot attribute. I could see a system where apart from accepting the client/bot instance in extensions' setup function, users could define extra arguments for the function of arbitrary type, which would be passed the relevant objects from the call to .load_extensions (probably as *args). I reckon this would go into a separate PR but I am mentioning this here as it'd likely depend on this PR.

That would be useful, yeah.
I'm sure people have different ideas for this, so it would warrant some more discussion about the specific design and to make sure common use cases are supported:

  • pass arguments as dict or **kwargs to setup?
    • perhaps even parse the signature to only pass the declared subset of kwargs?
  • how would it work with load_extensions, where the modules are not necessarily known ahead of time, and one may want to pass different arguments to different extensions?
  • ...

(created a thread here: https://canary.discord.com/channels/808030843078836254/1092780323307257906)

@shiftinv shiftinv added the t: refactor/typing/lint Refactors, typing changes and/or linting changes label Jun 22, 2023
@shiftinv shiftinv mentioned this pull request Mar 25, 2024
Open
3 tasks
@shiftinv shiftinv linked an issue Mar 25, 2024 that may be closed by this pull request
The load_extensions function is not working correctly #1177
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Enegg Enegg Enegg left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
s: needs review Issue/PR is awaiting reviews t: deprecation Deprecation of existing features t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
disnake
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The load_extensions function is not working correctly
3 participants
@shiftinv @Enegg @onerandomusername