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(telegram): add support for chat topics #1125

Closed
wants to merge 4 commits into from

Conversation

jon4hz
Copy link
Contributor

@jon4hz jon4hz commented Mar 12, 2024

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

@jon4hz
Copy link
Contributor Author

jon4hz commented Mar 17, 2024

As it seems github.com/crazy-max/gonfig doesn't support multidimensional arrays and I'm getting an error FTL Cannot load configuration error="failed to decode configuration from file: unsupported simple value type: slice".
So I did a little refactor and chatTopics is now a simple array, allowing you to define only one topic per chatID.

@jon4hz jon4hz marked this pull request as ready for review March 17, 2024 16:53
@jon4hz jon4hz requested a review from crazy-max as a code owner March 17, 2024 16:53
go.mod Outdated Show resolved Hide resolved
Copy link
Owner

@crazy-max crazy-max left a 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!

Comment on lines 105 to 108
var chatTopic int64
if len(chatTopics) > i {
chatTopic = chatTopics[i]
}
Copy link
Owner

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

Copy link
Contributor Author

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!

@ugorur
Copy link

ugorur commented Oct 14, 2024

did you stop working on this?

@bytebone
Copy link

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.

crazy-max

This comment was marked as off-topic.

@jon4hz
Copy link
Contributor Author

jon4hz commented Dec 17, 2024

Hey, so unfortunately I don't use diun anymore, so I didn't pursue this PR anymore. The next step here is a design decision, and that's up to @crazy-max.

@bytebone
Copy link

bytebone commented Dec 17, 2024

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

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.

Add support for Telegram group topics
4 participants