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

feat(commands.cog): pass errors raised by cog_unload to the bot's on_error #1067

Merged
merged 15 commits into from
Aug 20, 2023

Conversation

Snipy7374
Copy link
Contributor

Summary

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 pdm lint
    • I have type-checked the code by running pdm 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 shiftinv added t: enhancement New feature s: needs review Issue/PR is awaiting reviews labels Jun 25, 2023
@shiftinv
Copy link
Member

This would be a breaking change, since the exception info in this case is no longer available through sys.exc_info() as documented. The exception handler here could use a logger directly, like e.g. CommonBotBase.close

@Snipy7374
Copy link
Contributor Author

Snipy7374 commented Jul 11, 2023

This would be a breaking change, since the exception info in this case is no longer available through sys.exc_info() as documented. The exception handler here could use a logger directly, like e.g. CommonBotBase.close

so if you don't have a logger setted up then you don't see the error?

the original issue was a feature requesting to pass the error in the on_error event, if we use a logger to log the error should we also pass the error in the on_error? even if it prints NoneType: None or should we pass it in the on_error but ignore it when printing errors?

@shiftinv
Copy link
Member

so if you don't have a logger setted up then you don't see the error?

The root logger uses the WARNING level by default, so log.error and log.warning show up unless manually disabled

the original issue was a feature requesting to pass the error in the on_error event, if we use a logger to log the error should we also pass the error in the on_error? even if it prints NoneType: None or should we pass it in the on_error but ignore it when printing errors?

passing it into on_error would be weird/inconsistent, since it's meant for errors related to events. I'd say just logging it here is fine

changelog/1046.feature.rst Outdated Show resolved Hide resolved
Co-authored-by: shiftinv <8530778+shiftinv@users.noreply.github.com>
Signed-off-by: Snipy7374 <100313469+Snipy7374@users.noreply.github.com>
@shiftinv shiftinv enabled auto-merge (squash) August 20, 2023 15:39
@shiftinv shiftinv merged commit 18cc029 into DisnakeDev:master Aug 20, 2023
22 checks passed
@shiftinv shiftinv added this to the disnake v2.10 milestone Aug 27, 2023
@Snipy7374 Snipy7374 deleted the feat/cog_unload_err branch October 11, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review Issue/PR is awaiting reviews t: enhancement New feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

cog unload error logging
2 participants