Add the ability to send markdown messages

[irazasyed]

Add the ability to send markdown messages

[irazasyed]

Hey guys,

So since we can see Telegram keeps pushing changes every now and then which is breaking this library and effecting us all with our releases due to them not having any proper release cycle or following any standards. I think its about time we conclude to one of the available options we have in hand. As @jonnywilliamson already said, We had this discussion few months ago as well because of this very same reason of Telegram making changes at random times and making things difficult for us.

Hence, We have 3 options (From based on your ideas as well as mine) and I'm open for your inputs (This applies to everyone including the ones who've not participated in this thread).

Following are the options that we can go with and their pros & cons:

1. We keep appending the params as and when they're added by Telegram.
   +Point: This is definitely easier for us to make changes and to keep up with Telegram's unpredicted random changes they make and for us to push them right away without having to worry about breaking anyones bot (This doesn't apply for all the cases).
   -Point: The library will still break if one of their params is removed from anywhere or they make the param that we appended as the required param (In which case, The developer is forced to either use the other params or add nulls/whatever only to add the required value to the param we just positioned at last). Not something we can rely on and i can see we might have to reposition all the params once again if this happens causing it to break again and probably another discussion out of frustration of all these changes breaking our bots/library. Besides these things, The Developer now has to educate himself about this library in depth and can't simply just get started right away after referring the official docs. The dev is now expected to go through the library docs to understand each and every params position and how everything works whereas he could've just started and assumed the params position based on official docs.
2. We simply switch to the array method where the dev is expected to pass the params as an array to the method and those can be anything. What we can also do is, Have a checker in place to throw an exception which would simply check if the minimum required params are present for that method or not. Ex: Chat ID and Message for sendMessage the library would need at least these params to make an API request properly.
   +Point: We no longer have to worry about Telegram's random param changes, Whether they add a new one or remove anyone or reposition wherever they like. The library will still continue to function properly and we wouldn't be breaking anyones bot. And the devs need not worry about learning library docs fully or keep referring it every now and then. As long as they refer the official docs and they wanna use some param, they would just pass it to their params array. And we would only have a major release only when its really a breaking change (Either due to Telegram's major changes or due to the library's internal changes -- Though we'll still try our best to maintain backwards compatibility).
   -Point: Building an array which as compare to the current method might not be something you wanna do, Since you now are expected to give the option name and value whereas now you simply supply a value to the method which then builds an array behind the scenes. Besides this, The current bot developers using this library would have to make major changes in their codes to adapt this major breaking change but it would be one time hopefully.
3. We keep pushing changes as and when they're made while keeping the same method we use (Without a major release) and just probably follow a proper release cycle for ourselves (Like maybe only once a month we push a major new version at a given date) and the devs can come back on that date or later to see what breaking changes have been made and accordingly update their codes if they want.
   +Point: We wouldn't be breaking any bots and keep the library updated & would be following a proper release cycle. Also, Any breaking changes within this period (before the major release is tagged) would all be covered (So no matter how many changes Telegram makes in this period, We'll be able to support them all) and any other changes would automatically go under next release.
   -Point: One new param is added, removed or repositioned (Or some change that could break the library, even though its a very small one from Telegram's side), The devs would have to wait until the next major release/tag of this library (Though they can still use master branch but that isn't recommended for obvious reasons).

So that's about it. I can only think of these options and their positive/negative points right now. If i think of any more, I'll post them again here. I'll let you guys pick one option from these as well as give your inputs and maybe more ideas if any.

Thanks!

[jonnywilliamson]

Ok, I know my idea was crazy, I just wanted us to have the highest version number in packagist....heheh

Anyway, I'm only in favour of number 2 IF we can also improve the doc blocks at the same time. It drives me mad having to refer to documentation for the names of parameters when I'm in the middle of coding. Really slows me down. Having it visible in the IDE or whatever is a real boost to productivity.

So I had a play around with what we could do and I came up with this using some features of docblock I hadn't seen before. Let's use the sendmessage method as an example.

Changing the docblock to this:

Allows full documentation when coding:

If I had this I would be happy with option 2. I think that looks clean and easy to understand.

[irazasyed]

@jonnywilliamson

Like this:

[jonnywilliamson]

Firstly Telegram are awful for creating breaking changes.

The original sendMessage Method they create was like this:

https://github.com/irazasyed/telegram-bot-sdk/blob/master/src/Api.php#L234-L257

public function sendMessage(
        $chat_id,
        $text,
        $disable_web_page_preview = false,
        $reply_to_message_id = null,
        $reply_markup = null
    ) 

Now they want to do this:

public function sendMessage(
        $chat_id,
        $text,
        $parse_mode,
        $disable_web_page_preview = false,
        $reply_to_message_id = null,
        $reply_markup = null
    ) 

If we add this feature as it stands it will break everyone's code (or those who had based their code on the order of parameters in the first example).

@irazasyed wasn't happy to do this as he had just pushed v1.0 only 12 hours before this change came out.

As it stands he hasn't made a decision to either go with v2.0 and include the new parameter - but run the rish of Telegram changing parameter order again willy nilly at the drop of a hat. Or do a completely different approach of only supplying an array to the method making parameter order a non issue.

Also, it does also say:

For the moment, only Telegram for Android supports this.

So it seems like a feature that isn't really all that well supported.

[filipekiss]

It works either for Android and for Desktop, as I just tested with my bot (Screenshot included). Also, @irazasyed could just add the parameter as the last option to prevent breaking anything, at least to give us some way to use that. parse_mode is an optional argument, so

public function sendMessage(
        $chat_id,
        $text,
        $disable_web_page_preview = false,
        $reply_to_message_id = null,
        $reply_markup = null,
        $parse_mode = null
    ) 

would work just fine without breaking changes. (I actually did this to try it out and I can make a pull-request if you guys think it's worth to give the users this option).

[jonnywilliamson]

Oh I already offered the PR with the parameter at the end, had the chat with him back on September 9th 2015!!

I'll leave it to @irazasyed to decide what he wants to do.

[filipekiss]

Ok, no problem 😄

I'll just extend the Api class to add the parameter for now :)

[irazasyed]

We could also just keep doing major releases for every change that is breaking this library but that means, The devs would now have to keep updating their composer files and incrementing the major version number. While this is convenient for me, I'm sure it isn't something you wanna do that just to get that one param update. Besides this, There's a high chance we'll hit big version numbers in a very short period of time just to keep up with those simple param changes which technically are actually minor but for library it has become major. I see no point in following SemVer in such case TBH.

[jonnywilliamson]

Heres an odd ball idea.

How about using the change log date as our release number?

Here's what I mean: This is the current release/changelog:

Etc etc.

What if we said that our version numbers now match the release date? So instead of v1.0.1 we would have using the dates above:

v2015.10.08
v2015.09.18
v2015.09.07
v2015.08.29
v2015.08.15

So what if we don't have v3 or v4 and jump 2000 major points. It's only numbers - there's plenty more where they came from.

The ONE MAIN rule would be simple, the version you download from this repository will support exactly the relevant change-log from the Telegram website.

So if the version of the API here says 2016.01.22 but Telegram says the latest change was on 2016-Feb-02 then the developer knows that there may be features not implemented/or have been changed.

Bit of a crazy idea, I haven't ever seen it done...but any thoughts?

As for your worry about updating composer...well in this case you'd only have to do it once a year ie in composer ~2016

Otherwise I like idea 3.

[irazasyed]

Well, The idea is indeed crazy and I've honestly never seen this being done in any projects nor done it myself. My only concern is with the composer, How would it handle these changes/updates?

And what happens in case there is an internal change with the library that has got nothing to do with Telegram? For example it could be a major change, minor, patch/bugfixes, etc.

It's a bit odd to have that kinda versioning and not really a standard (We'd end up following Telegram devs, Since they don't seem to follow any standards, We too would be on their path which TBH i don't want to go their ways).

Lmk your thoughts on this.

Also, I'd vote for Option 2 with a combination of Option 3 (Have a proper release cycle, Any major changes internally or from Telegram would be pushed once a month only, I see this kinda release cycle being followed in a few projects).

[ihoru]

@irazasyed, you may create a poll (google sheets) and post link here to let us show what way of further development most preferable for library users (a kind of opinion statistic).
As for me, I like option 2 the most. In this case it would be possible to use features as fast as Telegram release them without many troubles.

[filipekiss]

@irazasyed The combination of Options 2 and 3 seems to be the one that may affect more users on a first update, but will make it easier to keep up-to-date with Telegram crazy changes and keep the library up-to-date. I don't see the need to use the dates as release versions, even tough I guess composer wouldn't have problems with it, but any changes made to the library that has nothing to do with Telegram API would add a layer of complexity to the version number (For example, version v2015.10.08-01 to add some library change but no changes from Telegram);

[filipekiss]

Just realised that the array options will also allow the parameter changes to work without the need to update the library.

Instead of having

public function sendMessage(
        $chat_id,
        $text,
        $disable_web_page_preview = false,
        $reply_to_message_id = null,
        $reply_markup = null
    ) {
        $params = compact('chat_id', 'text', 'disable_web_page_preview', 'reply_to_message_id', 'reply_markup');
        $response = $this->post('sendMessage', $params);

        return new Message($response->getDecodedBody());
    }

You'd have

public function sendMessage($params) {
        if (!is_array($params)) {
            //Throw some Exception
        }
        $response = $this->post('sendMessage', $params);

        return new Message($response->getDecodedBody());
    }

So when a new parameter is added all the developer has to do is add it to his array. He won't need to wait for a library update.

[irazasyed]

@ihoru Alright, Will do that soon.

@filipekiss When i say combination, I actually meant to have the major release tagging when there's actual major change from either Telegram's end or library itself (Not when the params are changed). All other minor changes would be pushed as and when they're made. The array method as you've already figured out would cut down our requirements of tagging the releases when Telegram makes these param changes and the library would be up to date in cases of these param changes. It'll give more control to the developer and at the same time resolve our problem with Telegram's crazy updates while following SemVer + Release Cycle.

[filipekiss]

@irazasyed Yes, I understand. I see no major problems with this approach except, as I said earlier, the first impact this would have on developers who need to change their code to match the new method signature. But still, the benefits seems to make up for that drawback, it would make the library stay closer to Telegram changes (at least when talking about parameters) and keep the versioning sane.

[irazasyed]

Yep! Indeed.

Here's the Poll Form: http://goo.gl/forms/qVIoMmtUpb please do your bit and submit your selection.

I'll keep this poll and issue open for a few days and then we'll take things forward from there. Thanks all.

[filipekiss]

@jonnywilliamson I have no objection to that docblock format :) It looks clean even if you don't use an IDE.

[irazasyed]

@jonnywilliamson wow that's actually a very nice docblock format. I'm cool with it. It's clean and understandable indeed. We can definitely do that.

[jonnywilliamson]

Cool I'm glad you like it. Then I'm ok with option 2 too!

Where does option 2 leave us though with having to add more code to each method to test that the REQUIRED parameters have been put into the array?

It seems like for every function we will have to write a special check for the required parameters for that method. That doesn't seem great (fun).

[filipekiss]

Doesn't the API check for it already? if i try to send an empty message I get an error already, so I'm pretty sure we can leave that to the API itself.

[jonnywilliamson]

These big decisions are why @irazasyed gets paid the big bucks...

[irazasyed]

We could do something like a simple if/else code. I'd wrap that into a helper function for that. Maybe something like:

We could put this into each method:

public function sendMessage($params) {
        if( ! $this->isRequiredParamsOk($params, ['chat_id', 'text']) ) {
             // Throw exception if these params are not supplied
        }

        $response = $this->post('sendMessage', $params);

        return new Message($response->getDecodedBody());
 }

Or just have this in the post method itself (As an optional param).

Something like this:

public function sendMessage($params) {
        $response = $this->post('sendMessage', $params, ['chat_id', 'text']);

        return new Message($response->getDecodedBody());
 }

So the post method itself would check for required params before making a request and if there is none passed (required params array to the post method to check), then it would do nothing. And if the array is passed and required params are not there, then it would throw an exception and maybe end execution of the script/return error.

@filipekiss API itself should be doing that yes, But why make unnecessary calls when the library could be a lil bit smarter out of the box? Would also make it easier to debug (Unless Telegram has a proper error code for missing params which we could use it to throw a new exception and then catch it), it isn't good from what i know from my last tests.

[irazasyed]

Update

I think 10 days was enough time for people to participate in poll. Since everyone is okay with the array method as per the poll results, Will go with the same and start making the appropriate changes over the next couple of days and push them the same.

Any contribution with regards to this change would highly be appreciated as always. Thanks!

[irazasyed]

So i was playing around with the code and came up with this new idea!

What do you guys think about it?

Basically, A simple object we pass to the API methods and as you soon as you press s when building the object, it'll show up all the available list of methods for that object to set the appropriate value. Those are btw not actually any real methods, those are magic methods.

If not an object, then phpdoc blocks (Few styles):

1.

    /* ... 
     * @param array $params [
     *                      int|string $chat_id,
     *                      string $text,
     *                      string $parse_mode,
     *                      bool $disable_web_page_preview,
     *                      int $reply_to_message_id,
     *                      string $reply_markup
     *                      ]
     * ... 
     */

2.

    /* ... 
     * @param array    $params
     *
     * @var int|string $params[chat_id]
     * @var string     $params[text]
     * @var string     $params[parse_mode]
     * @var bool       $params[disable_web_page_preview]
     * @var int        $params[reply_to_message_id]
     * @var string     $params[reply_markup]
     * ... 
     */

3. @jonnywilliamson's style - here

Let me know your inputs.

[ihoru]

For me better to use arrays (2nd style).
But for newbies much better to pass objects, I thinks.
So if you like objects so much, we can implement both variants...

[irazasyed]

I like the arrays too, With the 2nd style. Objects method is nice no doubt but we now would be creating an object class for each method in API, Seems like a lot of work and to maintain those is another thing!

Edit: No, I don't like objects so much, That was just another idea and lets stick to one idea and not have multiple variants, that'll confuse people i believe!

Lets see what others have to say about it :)

[filipekiss]

I like both methods and I'm fine with either, but if we choose the object method couldn't we use the Telegram Objects we already have here? It would require some changes, maybe, but it wouldn't be such a big deal and would give the users the opportunity to extend these objects with methods to help their specific needs over the array params.

[irazasyed]

We could use those yes! But the problem is, They extend BaseObject class which extends Laravel Collections class and requires array data to be passed when initializing the the object. That has to be modified and a few other things. How can it be more useful for the user though (Any proper use case)?

Edit: Since it already has methods related to those objects, Wouldn't that confuse people (As you type, it would show a list of get methods)?

[filipekiss]

Ah, yes... Since they extend the Laravel Collection all those methods would show as auto-completing and could cause quiet a confusion.

The extensions I mentioned are pre-formatted messages based on a set of attributes or things like that. I have a personal bot I use to send me notifications from a service called Sonnar and I could have a method to build the message based on the payload received from the service. But that could probably be worked around with the method returning the message array instead of setting properties. It's not a big deal, I was just thinking about personal uses I had, but the cons seems to outweight the pros. The array options seems to be the more versatile.

[irazasyed]

Oh i see. Well, People can still write their own classes to build the array with all the params (while connecting to other services or whatever) and still pass it on to the method. Like you've already mentioned. So the ideal solution is to go with the array option only. 👍

[jonnywilliamson]

Ok here's my 2 cent.

If we are changing, I'll be happy to stick with the array as parameter.

I also would like to stick with my doc block formatting....for one simple, yet time saving reason:

When you are creating the method, you can bring up the doc block, select the text and copy and paste the entire demo array very quickly! It's very easy then to delete the end of a line/replace it with your data.

With the other doc blocks

Copy and pasting either of those doc blocks doesn't help you much at all. You have to do loads of work to make them into a proper php array.

I'm not saying I do this alot, I don't, I'm saying why not keep that benefit?

[irazasyed]

@jonnywilliamson I get it. We can do both actually!

The parameters list (2nd style) could be used as a reference and when generating docs (So it lists all params properly), it'll pick up those nicely and on top of that, we could have your style of code. And in place of value, instead of the type right now (int/string/bool), we can leave that with just quotes.

Something like this:

$params = [
    'chat_id' => '',
    'text'    => '',
    ...
];

What you think?

[jonnywilliamson]

100% happy with that.

[irazasyed]

Cool, Will apply these then!

[jonnywilliamson]

Sorry I can't help this evening...maybe tomorrow

[irazasyed]

@jonnywilliamson No worries. I'm not pushing right now anyway. I'll be making these changes sometime this week. Any PR is also welcome.

[irazasyed]

Update

I've pushed the changes to the master branch. Anyone willing to give it a shot, feel free to do so and leave your feedback.

Note: You need to set the minimum-stability to dev in your composer file and then require for 2.0@dev version in order to pull in the master branch updates.

[jonnywilliamson]

You can't help yourself can you! When it's on your mind you just have to finish the job. Hahah.

Ok testing now.

[jonnywilliamson]

@irazasyed - Your note about needing to change the minimum-stability is not correct.

You can just require this package using composer with the following line:

composer require irazasyed/telegram-bot-sdk:dev-master

We really shouldn't edit the composer.json manually at all. Using the CLI is much safer.

[irazasyed]

@jonnywilliamson

You can't help yourself can you! When it's on your mind you just have to finish the job. Hahah.

Haha very true! Started with one and ended up finishing all 😆

Your note about needing to change the minimum-stability is not correct.
...

Well i keep forgetting we could do that. Also the last time i tried pulling in the master branch, it didn't let me do that because of minimum-stability (May it was set to stable that time). Thanks for the info!

[irazasyed]

P.S I'm closing this ticket now. Anyone who tests it and or finds any issues, Please create a new issue. Thanks all 👍

[jonnywilliamson]

Is anyone using Phpstorm 10?

Looks like there is a regression in the docbloc formatting. It now looks like this:

If I open it in Phpstorm 9, it looks lovely:

Very annoying after our hard work. Anyway, that's a side issue.

[irazasyed]

I'm using PHPStorm 10 and it's fine! Check your settings and or theme maybe?