Skip to content

Add error logging for config setup#431

Merged
MattyTheHacker merged 29 commits intomainfrom
config-error-notifications
Apr 13, 2025
Merged

Add error logging for config setup#431
MattyTheHacker merged 29 commits intomainfrom
config-error-notifications

Conversation

@MattyTheHacker
Copy link
Copy Markdown
Member

No description provided.

@MattyTheHacker MattyTheHacker linked an issue Mar 1, 2025 that may be closed by this pull request
@MattyTheHacker MattyTheHacker enabled auto-merge (squash) March 1, 2025 12:54
@MattyTheHacker MattyTheHacker self-assigned this Mar 1, 2025
@MattyTheHacker MattyTheHacker added the bug Something isn't working label Mar 1, 2025
@MattyTheHacker MattyTheHacker disabled auto-merge March 1, 2025 20:24
@MattyTheHacker MattyTheHacker marked this pull request as draft March 1, 2025 20:24
@MattyTheHacker MattyTheHacker marked this pull request as ready for review March 1, 2025 22:25
@MattyTheHacker MattyTheHacker enabled auto-merge (squash) March 1, 2025 22:25
CarrotManMatt
CarrotManMatt previously approved these changes Mar 31, 2025
Comment thread cogs/startup.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Comment thread config.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds error logging for configuration setup by integrating a Discord logging handler. The key changes are:

  • Import and usage of a DiscordHandler to log errors to a Discord channel.
  • Refactoring of the discord webhook setup function and its usage across configuration and startup.
  • Enhanced error logging during environment variable setup in config.py.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
config.py Refactored webhook setup for Discord logging and enhanced error handling.
cogs/startup.py Adjusted DiscordHandler instantiation to use dynamic service naming.
Comments suppressed due to low confidence (1)

config.py:181

  • [nitpick] The function name '_setup_discord_log_channel_webhook' is now inconsistent with the setting key 'DISCORD_LOG_CHANNEL_WEBHOOK_URL'. Consider renaming the function to better reflect its purpose, such as '_configure_discord_logging_handler'.
def _setup_discord_log_channel_webhook(cls) -> "Logger":

Copy link
Copy Markdown
Member

@CarrotManMatt CarrotManMatt left a comment

Choose a reason for hiding this comment

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

It might be nice to reorder the _setup_discord_log_channel_webhook() classmethod above all the other setup classmethods as this suggests it should get called before the other setup methods (as it does correctly in the _setup_env_variables() classmethod).

Entirely optional to do this, so have approved anyways (and disabled auto-merge to allow you to decide).

@MattyTheHacker MattyTheHacker merged commit d6f6c50 into main Apr 13, 2025
9 checks passed
@MattyTheHacker MattyTheHacker deleted the config-error-notifications branch April 13, 2025 12:15
@MattyTheHacker
Copy link
Copy Markdown
Member Author

It might be nice to reorder the _setup_discord_log_channel_webhook() classmethod above all the other setup classmethods as this suggests it should get called before the other setup methods (as it does correctly in the _setup_env_variables() classmethod).

Entirely optional to do this, so have approved anyways (and disabled auto-merge to allow you to decide).

Have merged because as far as i could tell it was already in the correct order? though some of the others aren't so if i can be bothered I'll fix the rest as it's own separate PR anyway

@CarrotManMatt
Copy link
Copy Markdown
Member

The call order is correct. I was referring to the definition order within the class. As in moving the _setup_discord_log_channel_webhook() definition to above the _setup_logging() definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor Improvements to the codebase that do not directly affect users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuration error does not send notification to discord

3 participants