Skip to content

Conversation

@mpforce1
Copy link

Description

Creates an abstract class EventHook for the log hook classes (Discord, Gotify, Matrix, Pushover, Telegram, and Webhook) and adds a list[EventHook] to the LoggerSettings. This enables users to specify as many hooks as required in their settings.

I've kept the old named hooks in the settings object to maintain backwards compatibility with existing user's settings.

This also potentially fixes a bug with the Pushover hook. The DROP_STATUS logs get combined into a single message for the other hooks, I think this got missed when the Pushover hook was written but nobody has made an issue about it so maybe it's fine.

I considered making the hooks into log handlers directly but that would have meant either inefficiently duplicating the work the log formatter does or having to rework the log formatter. It would also make it more difficult to combine those DROP_STATUS logs. This can still be done later if people think it would be better.

There is potentially another bit of code that could be abstracted, the hooks' send methods all first check if the given event is in the hook's self.events. This could be moved to the validate_record step to avoid having to redo this step for each hook. I've left it alone for now because I wanted to avoid touching the send methods if possible.

Finally, I think the reason Gotify.py looks like I've replaced the whole file is because it originally had windows line endings, I reformatted the file to use unix ones because git was complaining about it.

Fixes #704

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

I added multiple Discord hooks to the list with different event filters and saw the desired events went to the correct hooks. I also tested using the existing named hook alongside this and it also worked as expected. I did not test with the other hook classes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (README.md)
  • My changes generate no new warnings
  • Any dependent changes have been updated in requirements.txt

@Gamerns10s
Copy link

Ou of context request but ngl you seem like a person who actually understands the whole working of the script i would just like to request you if you also try to fix the drops progress bar feature. I tried fixing it using chatgpt and all that stuff but it didn't worked even made it an issue in this repo but I think the owner isn't active nowadays so maybe you could try to fix it if you're fine with it 😅

@mpforce1
Copy link
Author

Ou of context request but ngl you seem like a person who actually understands the whole working of the script i would just like to request you if you also try to fix the drops progress bar feature. I tried fixing it using chatgpt and all that stuff but it didn't worked even made it an issue in this repo but I think the owner isn't active nowadays so maybe you could try to fix it if you're fine with it 😅

I'm happy you think I have a decent knowledge of the codebase, unfortunately I don't really use the drops feature so my working knowledge of that isn't great. I'm currently working on something else for the project (#722) so I can't look into it now. Might be a while for me so your best bet is to try something yourself and see if you can get it working.

@Gamerns10s
Copy link

Oh alr no issues about that then but if you're working on something else rn that's great but then I have another request for you that instead of pushing changes into a pr here just make a fork of this and push into there cuz ngl this repo is pretty dead now and as of rn many people have proposed some fixes which still haven't been merged and some of them even just closed their pull requests because of this. With a new and active fork atleast the changes proposed by community would be merged. Again this is just a suggestion 😄

@Gamerns10s
Copy link

Oh alr no issues about that then but if you're working on something else rn that's great but then I have another request for you that instead of pushing changes into a pr here just make a fork of this and push into there cuz ngl this repo is pretty dead now and as of rn many people have proposed some fixes which still haven't been merged and some of them even just closed their pull requests because of this. With a new and active fork atleast the changes proposed by community would be merged. Again this is just a suggestion 😄

Even the last actual update with any new features and improvements was last year i believe

@mpforce1
Copy link
Author

Oh alr no issues about that then but if you're working on something else rn that's great but then I have another request for you that instead of pushing changes into a pr here just make a fork of this and push into there cuz ngl this repo is pretty dead now and as of rn many people have proposed some fixes which still haven't been merged and some of them even just closed their pull requests because of this. With a new and active fork atleast the changes proposed by community would be merged. Again this is just a suggestion 😄

I do have a fork that's the base for my PRs here. Anyone can make their own build using that as a base, there's no automated build system since it's only intended to be used for developing new features for this repo. You could also try making your own fork, implementing a fix, and making an upstream PR back to this repo. This would allow you to run the project using your own fix branch as a base. This is actually how I run the project as I like to use a custom prediction system as per #659.

Even the last actual update with any new features and improvements was last year i believe

The last update that fixed a major issue was 3 months ago, although I'll give you that this wasn't a feature update. The last feature update was almost a year ago.

Another option to help enable more frequent updates would be to add some project collaborators. This should enable any invited community member to make changes when the repo owner or other collaborators are unable to make time for it.

Adds an abstract base class for all log event hook classes.
Adds LoggerSettings.hooks to enable users to add as many of these as they need to their configs.
Updates LoggerSettings constructor type hinting to use "|" instead of "or".
Moves logger.py log validation functions into their respective hook class.
Abstracts the attribute based log validation into an intermediary hook class method.
Updates the README.md and example.py.
Potentially fixes a bug with how DROP_STATUS Events worked with Pushover.
@bossanovaorca
Copy link

@rdavydov is there anything left to address before this PR can be merged? would love to see this feature!

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.

Possible to create multiple Discord notification channels

3 participants