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

Publish to telegram #14

Closed
wants to merge 12 commits into from
Closed

Publish to telegram #14

wants to merge 12 commits into from

Conversation

sgaynetdinov
Copy link
Contributor

#13

@umputun
Copy link
Owner

umputun commented Nov 12, 2019

thx for PR.

A few things:

  1. Looks like you did go get -u and updated all deps? This is not a bad thing, but kind of unexpected
  2. Send shouldn't access env directly, this has to be handled by main's Opts and injected
  3. The bot seems to send item.Enclosure.URL. I have no clue how this handled and what will be posted, url? or file? It will be nice to see a screenshot of what it looks like in real life.
  4. In addition to file it should post the title and description
  5. Wiring telegram notification into the store is the wrong place to do it. The store is dealing with data storage only and the processing (including notification) should be handled on proc level
  6. FeedMaster handles multiple Feeds (see proc.Conf). Each feed may send to different telegram channels, so the channel should be moved from command-line opts / env to config.

…Opts and injected

The store is dealing with data storage only and the processing (including notification) should be handled on proc level
@sgaynetdinov
Copy link
Contributor Author

It will be nice to see a screenshot of what it looks like in real life.

uwp

radio-t

echo_msk

@sgaynetdinov
Copy link
Contributor Author

The bot seems to send item.Enclosure.URL. I have no clue how this handled and what will be posted, url? or file?

url.

here is a way around the limit 50MB, but I have not tried
#13 (comment)

@umputun
Copy link
Owner

umputun commented Nov 13, 2019

thx for screenshots, look fine. Not sure why it lost links from the show notes, probably converting it to markdown may help? Or this is upstram of smth in processing stripping links?

@umputun
Copy link
Owner

umputun commented Nov 13, 2019

btw, let me know as it ready for review. As of now it is shown as a "draft"

@sgaynetdinov
Copy link
Contributor Author

sgaynetdinov commented Nov 14, 2019

Not sure why it lost links from the show notes, probably converting it to markdown may help? Or this is upstram of smth in processing stripping links?

Fixed 0456fed show only link html tag.

Telegram not support all html tag and not support nested tag
https://core.telegram.org/bots/api#html-style


2

1

@sgaynetdinov sgaynetdinov marked this pull request as ready for review November 14, 2019 06:45
@sgaynetdinov
Copy link
Contributor Author

btw, let me know as it ready for review. As of now it is shown as a "draft"

Ready!

@umputun
Copy link
Owner

umputun commented Nov 15, 2019

btw, because of the massive changes in modules & vendor, this change will be really ugly if I squash commits. Can you, pls keep the current vendors and modules alone and just add the one you really need? I'll update deps and revendor after the merge if needed.

@sgaynetdinov
Copy link
Contributor Author

Can you, pls keep the current vendors and modules alone and just add the one you really need?

OK, how to do it right, create a new PR?

@umputun
Copy link
Owner

umputun commented Nov 15, 2019

yes, this probably the simplest way

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.

2 participants