Move exception parsing to extension package

[karb0f0s]

Suggestion: move exception parsing functionality to the extension package.

The Telegram API error messages are not part of the API, so they could change at any moment. That is why we cannot guaranty that code based on exceptions would work in changing circumstances. The library should throw only basic exceptions based on API response codes, such as ApiException, ForbiddenException, BadRequestException for general error codes, such as 400, '403, 429and such. To allow users handle different error codes we could provide separate packageTelegram.Bot.Exceptions`, that will return specific error code (object) that will help users handle different types of errors. This change would allow update only this package to keep up with API changes and keep core library functionality untouched.

try { bot.SendMsg() }
catch (ApiRequestException e) {
  switch (parser.Parse(e)) {
    case ParameterException:
      foo();
    case ChatBlockedException:
      bar();
  }
}

This change would mean that we will mark all minor exceptions as obsolete, and no more exception minor exceptions would be added to the library.

[tuscen]

I suggest using Telegram.Bot.Extensions namespace prefix for this (as well as other useful library helpers). I'm already using it for KeyboardBuilders and Filters packages.

[matteocontrini]

Honestly I don't like this approach...
I think it's much better to write

try { bot.SendMsg() }
catch (ParameterException ex) {
  foo();
}
catch (ChatBlockedException ex) {
  bar();
}

instead of

try { bot.SendMsg() }
catch (ApiRequestException e) {
  switch (parser.Parse(e)) {
    case ParameterException:
      foo();
    case ChatBlockedException:
      bar();
  }
}

from a developer perspective.
I understand that you're trying to keep the library as small as possible, but I think error handling is a basic feature for an API wrapper.

[tuscen]

@matteocontrini The problem is not the library size. If exceptions were a part of Bot API and were properly documented then it wouldn't be a problem. But the thing is that they are changed all the time by telegram devs, they're not documented so we have to hunt them down every time therefore we can't guarantee anything stable and it's just complicate things. I understand that the proposed solution is not ideal.

[poulad]

@karb0f0s I agree with making a separate package for exceptions.

@tuscen It's a good idea to have that naming convention for extension packages.

[poulad]

@matteocontrini is right and I might have a solution for it.

Interface for exception parser in client package:

// in package Telegram.Bot
interface IExceptionParser { ApiRequestException Parse(JObject response); }

User can optionally register an exception parser

// using Telegram.Bot.Extensions.Exceptions
var client = new TelegramBotClient(token) {
  ExceptionParser = new ExceptionParser()
}

And code looks like normal

try { bot.SendMsg() }
catch (ParameterException ex) {
  foo();
}
catch (ChatBlockedException ex) {
  bar();
}

[Olfi01]

Looks good. Also, I like poulads approach.

[poulad]

All right. it seems majority is OK with that. I create a repo for it.

[poulad]

Furthermore, we can run exception tests periodically (might be daily) using Travis-CI jobs to stay alert when tg changes some of its API behavior.

We might want to skip the test cases that require user interaction.

[poulad]

It's better to be verbose in documenting the exception classes because it isn't covered in bot API. A hierarchical representation of classes would be nice as well (Sandcastle maybe?).

[karb0f0s]

Proposed changes to Telegram.Bot library are in TelegramBots/Telegram.Bot#694 TelegramBots/Telegram.Bot#695.
I'd like to change specific class exceptionNameErrorCode and exceptionNameErrorDescription to general class properties ErrorCode and ErrorCodeDescription. Than it will be possible to lookup exception like this:

var exceptionTypeInfo = exceptions.FirstOrDefault(
  typeInfo => typeInfo.ErrorCode == apiResponse.ErrorCode
);

return Activator.CreateInstance(
    exceptionTypeInfo.exceptionType,
    apiResponse.Description
        .Replace(exceptionTypeInfo.ErrorCodeDescription, string.Empty),
    apiResponse.Parameters
  ) as ApiRequestException;
);

[karb0f0s]

I guess this extension point might lead to problems:

https://github.com/TelegramBots/Telegram.Bot/blob/675fc4b992b69398ddbc9d46c1efd3ea251653b1/src/Telegram.Bot/TelegramBotClient.cs#L279-L280

Here are hardcoded HTTP Errors, that we expect parser will handle:

https://github.com/TelegramBots/Telegram.Bot/blob/675fc4b992b69398ddbc9d46c1efd3ea251653b1/src/Telegram.Bot/TelegramBotClient.cs#L257-L268

But if we accept external parser, it should provide it's own HTTP Errors list, and this check might not have sense for external exception handler:

https://github.com/TelegramBots/Telegram.Bot/blob/675fc4b992b69398ddbc9d46c1efd3ea251653b1/src/Telegram.Bot/TelegramBotClient.cs#L257-L259

And this raise another question - should external parser only extend existing exceptions, or deliver all range of exceptions?

[tuscen]

@karb0f0s I think it's okay for exception parser to return any exception type. It seems that it will need to return not only ApiRequestException derived exception but also HttpRequestExceptions (e.g. 5XX exceptions). Therefore it will need to accept HttpResponseMessage instead of JObject.