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

Refactor "Cogs" cog #484

Merged
merged 19 commits into from Oct 11, 2019

Conversation

@MarkKoz
Copy link
Member

commented Oct 2, 2019

Notable user-facing changes:

  • Fixes #479
  • Embeds have been ditched everywhere except for the list command.
  • Renamed to extensions because it really loads extensions, not cogs (cogs just happen to be inside extensions). The old cogs aliases are still available.
  • * can be used to reload only all loaded extensions and ** can be used to "reload" all extensions, even unloaded extensions.
  • Multiple names of extensions can be given to the reload command. If * is also given, a union will be done with all loaded extensions and the extensions named explicitly.
  • The fully qualified name of the extension is shown in response messages (e.g. confirmation/failure messages, errors, etc.). The list command is an exception and shows unqualified names.
  • When batch reloading, errors have been merged into a single list (no more distinction between load/unload errors).
  • reload_extension() is used for reloading, so if an error occurs while reloading, the extension will be reverted to its previously working state.
  • When reloading, if an extension was not initially loaded, it will be implicitly loaded anyway.

Plan is to rename it to extensions and re-do it based on how I did it in another bot, which has less redundancy in terms of output messages:

https://github.com/MarkKoz/d.py-test-bot/blob/master/testbot/extensions/manager.py
https://github.com/MarkKoz/d.py-test-bot/blob/master/testbot/utils/extensions.py

MarkKoz added 14 commits Oct 2, 2019
The cog now keeps a set of full qualified names of all extensions.
The converter fully qualifies the extension's name and ensures the
extension exists.

* Make the extensions set a module constant instead of an instant
  attribute and make it a frozenset.
* Add a cog error handler to handle BadArgument locally and prevent the
  help command from showing for such errors.
* Store just the names rather than entire ModuleInfo objects
* Fix prefix argument
* Rewrite docstrings for commands
* Rename KEEP_LOADED to UNLOAD_BLACKLIST and make it a set
* Change single quotes to double quotes
* Add "cogs" as an alias to the extensions group
* Rename reload_all to batch_reload
* Simplify output format of batch reload with only 1 list of failures
* Show success/failure emoji for batch reloads
* Simplify logic in the manage() function
* Clean up some imports
* Rename accordingly from cogs to extensions
* Use the Extension converter
* Make the argument variable instead of keyword-only
@MarkKoz MarkKoz force-pushed the extensions-cog branch from 99455af to 82fb11c Oct 4, 2019
Copy link
Member Author

left a comment

The extension names are displayed to the user as fully qualified (though commands accept unqualified names). The only exception is the list command, which always displays unqualified names. Is this OK? It was just simpler implementation-wise but wouldn't be a lot of code to add it either.

bot/cogs/extensions.py Outdated Show resolved Hide resolved
except (commands.ExtensionAlreadyLoaded, commands.ExtensionNotLoaded):
if action is Action.RELOAD:
# When reloading, just load the extension if it was not loaded.
return self.manage(ext, Action.LOAD)

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 4, 2019

Author Member

Is implicitly loading an extension if it isn't loaded undesirable behaviour? If not, another candidate would be to implicitly reload when !ext load is done on an already loaded extension.

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 7, 2019

Member

I think that the intention of loading the cog is clear when someone tries to reload a cog, so I don't mind this behavior. The alternative proposal, reloading when someone tries to load something, sounds more counter-intuitive to me. If I recall correctly, the previous version with the misnomer cogs denied both, but I often found it annoying that it wouldn't just load an extension on reload when it wasn't loaded.

This comment has been minimized.

Copy link
@MarkKoz

MarkKoz Oct 7, 2019

Author Member

Yeah, I felt the same way with the second proposal. Leaving this unresolved for now in case anyone else has opinions to share.

This comment has been minimized.

Copy link
@lemonsaurus

lemonsaurus Oct 11, 2019

Member

I think it's fine, too.

bot/cogs/alias.py Show resolved Hide resolved
@MarkKoz MarkKoz marked this pull request as ready for review Oct 4, 2019
@sco1 sco1 added this to Needs review in Bot Tracking Oct 4, 2019
bot/cogs/alias.py Show resolved Hide resolved
bot/cogs/extensions.py Outdated Show resolved Hide resolved
except (commands.ExtensionAlreadyLoaded, commands.ExtensionNotLoaded):
if action is Action.RELOAD:
# When reloading, just load the extension if it was not loaded.
return self.manage(ext, Action.LOAD)

This comment has been minimized.

Copy link
@SebastiaanZ

SebastiaanZ Oct 7, 2019

Member

I think that the intention of loading the cog is clear when someone tries to reload a cog, so I don't mind this behavior. The alternative proposal, reloading when someone tries to load something, sounds more counter-intuitive to me. If I recall correctly, the previous version with the misnomer cogs denied both, but I often found it annoying that it wouldn't just load an extension on reload when it wasn't loaded.

bot/cogs/extensions.py Outdated Show resolved Hide resolved
bot/cogs/extensions.py Outdated Show resolved Hide resolved
MarkKoz added 3 commits Oct 7, 2019
* Rename batch_reload() to batch_manage() and make it accept an
  action as a parameter so that it can be a generic function.
* Switch parameter order for manage() to make it consistent with
  batch_manage().
* Always call batch_manage() and make it defer to manage() when only 1
  extension is given.
* Make batch_manage() a regular method instead of a coroutine.
@MarkKoz MarkKoz requested a review from SebastiaanZ Oct 7, 2019
Copy link
Member

left a comment

Looks good to me. I think I've tested everything.

Copy link
Member

left a comment

solid

@jchristgit jchristgit merged commit 20f815a into master Oct 11, 2019
2 checks passed
2 checks passed
Bot Build #20191009.12 succeeded
Details
Bot (Lint & Test) Lint & Test succeeded
Details
@jchristgit jchristgit deleted the extensions-cog branch Oct 11, 2019
@jchristgit jchristgit self-assigned this Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bot Tracking
  
Needs review
5 participants
You can’t perform that action at this time.