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

Rework extension loading #1423

Merged

Conversation

baronkobama
Copy link
Contributor

@baronkobama baronkobama commented Jun 15, 2022

Summary

This PR allows for folders to be loaded (allows for recursing through folders as well) in load_extension and adds the helper method load_extensions that allows for loading multiple extensions in one function call.

Closes #946

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • 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, typehinting, examples, ...)

Notes

  • I've tested this PR myself and haven't found any issues but any additional testing would be much appreciated.
  • Credit to @Sodynoizz for a decent portion of the code written here. Assuming this PR looks good, then it will replace Add load_extension_from_path function #1413.

@Sodynoizz
Copy link

Should we change load_extensions, load_extension , reload_extension and unload_extension to asynchronous function?

@OldPranoy

This comment was marked as spam.

@Sodynoizz

This comment was marked as resolved.

@OldPranoy

This comment was marked as spam.

@krittick

This comment was marked as resolved.

@Lulalaby Lulalaby enabled auto-merge (squash) June 15, 2022 19:02
@baronkobama baronkobama disabled auto-merge June 18, 2022 23:12
@baronkobama
Copy link
Contributor Author

baronkobama commented Jun 18, 2022

PR isn't ready for merge just yet, recursive loading doesn't appear to work currently.

@Lulalaby Lulalaby force-pushed the rework-extension-loading branch from 8c2e519 to 31c7eb3 Compare June 18, 2022 23:20
@Lulalaby Lulalaby marked this pull request as draft June 18, 2022 23:20
@baronkobama
Copy link
Contributor Author

Ready for review again, hopefully, bug-free this time around.

@baronkobama baronkobama marked this pull request as ready for review June 18, 2022 23:53
plun1331
plun1331 previously approved these changes Jun 19, 2022
Copy link
Member

@plun1331 plun1331 left a comment

Choose a reason for hiding this comment

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

Looks good, I would suggest adding a parameter to ignore (or just print) extension loading errors if multiple extensions are being loaded, or some other solution that doesn't raise an exception and stop extensions from being loaded.

Additionally, we could consider potentially unloading loaded extensions if one fails.

@baronkobama
Copy link
Contributor Author

Looks good, I would suggest adding a parameter to ignore (or just print) extension loading errors if multiple extensions are being loaded, or some other solution that doesn't raise an exception and stop extensions from being loaded.

The first implementation of this idea is done in b587b10, please take a look when you get a chance. No parameter to change this behavior is added currently but I can add it later down the line if still recommended.

@baronkobama
Copy link
Contributor Author

No parameter to change this behavior is added currently but I can add it later down the line if still recommended.

This is now implemented in 9e377e7. So far, I've extensively tested the new loading and haven't found any issues. However, due to the addition of numerous new features including a new helper function, recursive loading, and a new parameter, store, any additional testing would be much appreciated.

@baronkobama
Copy link
Contributor Author

Currently mulling this over and I'm wondering about one last thing before this PR should be fully ready for review/merge: should load_extension be returning the isolated cog name like it currently is or should we be returning the full name that was passed? I'm not sure of this myself but I'm leaning towards returning the full name. Any feedback on this would be much appreciated.

@Lulalaby
Copy link
Member

I would prefer full name too tbh

@Dorukyum what you think

@Dorukyum
Copy link
Member

I would prefer full name too tbh

@Dorukyum what you think

I agree, it should return the full name.

@Lulalaby Lulalaby enabled auto-merge (squash) June 29, 2022 00:43
Copy link
Member

@Dorukyum Dorukyum left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Lulalaby Lulalaby merged commit 5a42fff into Pycord-Development:master Jul 2, 2022
Sodynoizz added a commit to Sodynoizz/pycord that referenced this pull request Jul 3, 2022
@vproyaev
Copy link

On line 856 the check is not correct, you check final_out -> it's dict in isinstance(final_out, Exception) it's wrong. Need change to isinstance(final_out[name], Exception)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow directories to be passed in load_extension
8 participants