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

New notifier: Discord + minor fixes #486

Merged
merged 8 commits into from
May 1, 2024
Merged

Conversation

lars-devs
Copy link
Contributor

@lars-devs lars-devs commented Mar 31, 2024

I'm running a cluster of three TGTG instances on different servers that each takes care of different weekdays to prevent rate limiting. To add an item to them, I need to connect via SSH three times. The telegram notifier already allows to add favorites, but there's still need to message three bots. That's why I created the notifier for Discord, so these three instances can join a Discord server at the same time and listen to commands for adding favorites - either by having different command prefixes, or all the same, so there's only need to type in the command one single time.

The bot features all the Telegram commands with exception of reservation functionality. This may be included in the future. With !gettoken and !getinfo (prefix in my case is an exclamation mark), it's possible to show connection information.

grafik

Note about test warnings: I implemented test_discord() in test_notifiers.py. Since discord.py imports module audioop which is about to be deprecated in later Python releases, this warning appears:

.venv/lib/python3.11/site-packages/discord/player.py:28
  /workspaces/tgtg/.venv/lib/python3.11/site-packages/discord/player.py:28: DeprecationWarning: 'audioop' is deprecated and slated for removal in Python 3.13
    import audioop

If there's a way to avoid this warning, please let me know.

Pull Request Checklist

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Did you make your Pull Request on the dev branch?
  • Does your submission pass tests? make test
  • Have you lint your code locally prior to submission? make lint
  • Could you build and run the docker images successfully? make images
  • Could you create a running executable? make executable
  • Have you added an explanation of what your changes do and why you'd like to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran manual tests with your changes locally?

@Der-Henning
Copy link
Owner

Hi @lars-devs.
Thank you for your contribution.
I will have a look at your code shortly.
Please check pre-commit. Your poetry.lock file seems to be out of date.

@lars-devs
Copy link
Contributor Author

lars-devs commented Apr 1, 2024

Hi @Der-Henning, somehow make lint doesn't update the poetry.lock file for me. I was able to regenerate the file by renaming it and running make lint again. Is there a specific command that I can run to force pre-commt to run instead or was renaming sufficient?

I just found out, the test routine will fail with the example login data which isn't really suprising, sincen their invalid. I'm wonderin why the test of Telegram runs through, though, even with the catched exception of telegram.error.InvalidToken. Discord and Telegram notifiers work nearly the same. Maybe you

btw, I implemented formatting price, value, and rating to match localized conventions (de_DE: 1,50 EUR, en_US: 1.50 USD). Do You want me to create another PR or is including this into the current PR fine?

@Der-Henning
Copy link
Owner

Hi @lars-devs.
Using the default configuration with only adding the bot token results in DiscordConfigurationError as the channel is set to 0. Should the bot identify the channel by itself or please add a description to obtain the channel id?
The discord notifier test fails on discord.bot.dispatch("close") with RuntimeError: Event loop is closed. I ran the test on python 3.11.

@lars-devs
Copy link
Contributor Author

Hi @Der-Henning, maybe there's a misunderstanding on my side. Do You run the make test only with valid data or with the example values (123, ABC, ...)?

The channel is set in line 332 in test_notifiers.py to an example value. The exception occurs because of an invalid token, which is the case for the Telegram test as well. That's why I'm wondering how the Telegram test runs through even with default data. But if the make test isn't meant to be executed with example, invalid information, than there's nothing wrong here.

Should the bot identify the channel by itself

I added information about it to the wiki.

Also, could You please check the two other points I mentioned (make lint, formatting prices)? Thanks!

@Der-Henning Der-Henning merged commit ac37055 into Der-Henning:dev May 1, 2024
18 of 20 checks passed
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.

None yet

2 participants