-
-
Notifications
You must be signed in to change notification settings - Fork 122
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(telegram): add support for chat topics #1125
Conversation
As it seems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rebase with #1135 merged? Also see my review, thanks!
internal/notif/telegram/client.go
Outdated
var chatTopic int64 | ||
if len(chatTopics) > i { | ||
chatTopic = chatTopics[i] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should use a second array to define topics. That's error prone imo.
I think we could either use a separator in chat ID to define topics like 123456789:25
where 123456789
is the chat ID and 25
an optional topic/thread id.
Also I think a chat ID can have multiple topics right?
If that's the case we could either define comma separated list of topic IDs in chat ID like 123456789:25,12
or keep ChatTopics
that would be a map like:
chatTopics:
123456789:
- 25
- 12
I would tend to reuse chatIDs with 123456789:25,12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could either use a separator in chat ID to define topics like 123456789:25 where 123456789 is the chat ID and 25 an optional topic/thread id.
Unless I'm misunderstanding something, that would mean we'd have to change the chatID to a string, right? That would be a rather breaking change and I'm not sure if that is the best solution.
I made a draft of using a map for the chat topics in 1915c27. It's working but it has the downside that you have to make the map keys explicitly a string (github.com/crazy-max/gonfig
doesn't support int64
as map keys).
Let me know what you think!
did you stop working on this? |
As it stands now, I guess that feedback from crazy-max is required to go forward with this. I am highly interested in this am able + willing to contribute where necessary, should jon no longer be available. |
Hey, so unfortunately I don't use |
Thank you jon for your work so far, allow me to pick it up from your last commit. I'll prepare a rebased fork and make a new PR, and we can continue the work there. Edit: Created at #1305 |
This PR adds support for telegram chat topics.
github.com/go-telegram-bot-api/telegram-bot-api
doesn't have support for topics despite several open PRs, so I switched it out with a more up-to-date library.Diun supports sending messages to multiple chats so
ChatTopics
is a 2d slice, which allows diun to send messages to multiple chats and multiple topics.I couldn't quite figure out how the environment variables are parsed, so I'm not 100% sure if the parsing of multi dimensional arrays works. I'll test that before I remove the draft status.
This is my first time contributing to
diun
so please let me know if I overlooked something or if have any other comments so far!Closes #948 and #1031