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

Hi, there not really PR, but new vision of tgbotapi with new API #29

Merged
merged 33 commits into from Nov 22, 2015
Merged

Conversation

zhulik
Copy link
Contributor

@zhulik zhulik commented Nov 21, 2015

Many refactorings and DRYing, all Send* functions replaces with one Send(), Some other small changes. Tests added, 85% coverage. Everything works. Travis added.

Please, check my code. I know, that public API is changed, but it's really useful. It will be great if it possible to merge my code to upstream, but i can rename project and develop it as my own.

Package Syfaro/telegram-bot-api in my fork changed to zhulik/telegram-bot-api. I will change it back if you will merge my code.

Thanks!

ps: I want to develop some Bot framework based on it, with commands support, sessions, middlewares and so on

@Syfaro
Copy link
Member

Syfaro commented Nov 21, 2015

Do you happen to know how many public methods changed?
I tried updating a project I'm working on, and it only took a few seconds to make it work, so it doesn't seem like it's incredibly different.

Only issue I can see right now is that golint spits out 50 things, mostly because there aren't comments on your new methods.

Also, what was the reasoning behind removing the tgutils package?

And lastly, I've been playing around with designing a bot framework, you can find it here.

@zhulik
Copy link
Contributor Author

zhulik commented Nov 21, 2015

@Syfaro SendMessage, SendPhoto, SendDocument, SendAudio, SendVoice, SendSticker, SendVideo, SendLocation, SendChatAction is replaced with one method Send.
UpdatesChan renamed to GetUpdatesChan, BotAPI has no more Updates channel in struct. Channel will returned from GetUpdatesChan and ListenForWebhook(examples in readme and bot_test.go). Some public methods added(Like RemoveWebhook). Anybody can update their apps in 5 minutes to use new API.

About tgutils package: i suppose, that encoding audio to correct format is not responsibility of bindings lib, furthermore it support only convertion to OGG, but telegram documentation wants mp3 for SendAudio and Ogg for SendVoice. So, i think that user should prepare audio with other tools.

About golint: i will fix it's issues tomorrow.

My framework will be here.

@zhulik zhulik closed this Nov 21, 2015
@zhulik zhulik reopened this Nov 21, 2015
@Syfaro
Copy link
Member

Syfaro commented Nov 21, 2015

Alright, cool. Not too many breaking issues, I'd love to merge it after you fix the golint stuff.

@zhulik
Copy link
Contributor Author

zhulik commented Nov 21, 2015

@Syfaro Lint issues fixed

@Syfaro
Copy link
Member

Syfaro commented Nov 21, 2015

Could you fix the package names now and links in the README?

After that, I'll merge it.

@zhulik
Copy link
Contributor Author

zhulik commented Nov 21, 2015

@Syfaro Hey, names fixed. Please register Travis CI and add your repository to it. Also, i hardcoded some File IDs, Chat IDs, Message IDs c and test Telegram Bot Token in bot_test, so i will receive messages from tests, it is not problem for me. Bot token is for test only and has opened chat only with me. I suppose there is no security issues.

In the future, i can replace real http requests with mocks, but not now

@Syfaro
Copy link
Member

Syfaro commented Nov 22, 2015

Registered it.

I don't mind the hardcoded stuff, I might change it to send to me instead at some point.

@zhulik
Copy link
Contributor Author

zhulik commented Nov 22, 2015

I think, everything is OK, you can merge it

Syfaro pushed a commit that referenced this pull request Nov 22, 2015
Major overhaul to make everything easier to use
@Syfaro Syfaro merged commit b034326 into go-telegram-bot-api:master Nov 22, 2015
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