Keyboard improvements + generics #209
Conversation
library/src/main/java/com/pengrad/telegrambot/model/request/InlineKeyboardMarkup.java
Outdated
Show resolved
Hide resolved
| return new Request.Builder() | ||
| .url(baseUrl + request.getMethod()) | ||
| .post(createRequestBody(request)) | ||
| .build(); | ||
| } | ||
|
|
||
| private RequestBody createRequestBody(BaseRequest<?, ?> request) { | ||
| RequestBody createRequestBody(BaseRequest<?, ?> request) { |
There was a problem hiding this comment.
let's keep it private.
I will explain my point below in TelegramBotClientTest
| } | ||
| return builder.build(); | ||
| } | ||
| } | ||
|
|
||
| String toParamValue(Object obj) { |
There was a problem hiding this comment.
also can be private, if we remove TelegramBotClientTest
| @@ -10,28 +13,49 @@ | |||
| public class InlineKeyboardMarkup extends Keyboard implements Serializable { | |||
There was a problem hiding this comment.
Tests in ModelTest fails because this class now.
There are 2 things:
- equalsVerifier fails, need to revert
o instanceof InlineKeyboardMarkupin equals() - Keyboard before implemented custom
toString, sotestToStringpassed, need to generate it for this class
|
|
||
| @Test | ||
| public void parseSendResponseBadRequest() { | ||
| String responseFromTelegram = "{\"ok\":false,\"error_code\":400,\"description\":\"Bad Request: message can't be edited\"}"; |
There was a problem hiding this comment.
this is probably same TelegramBotTest.testResponseParameters() and not related to TelegramBotClient.
I suggest to remove
|
|
||
| @Test | ||
| public void sendMessageBody() throws IOException { | ||
| Keyboard oldKeyboard = new ReplyKeyboardMarkup( |
There was a problem hiding this comment.
I think it's more "migration" test for new ReplyKeyboardMarkup.
And we anyway need to check it via integration test with real API (TelegramBotTest)
yeah, this test can't be launched now coz I used private properties, I will try to change this in future.
so I think we can close createRequestBody method to private and remove this class.
I really believe it's much useful to test all methods with real API.
and then client implementation details will be automatically covered.
what do you think?
|
|
||
| @Test | ||
| public void sendPollBody() throws IOException { | ||
| SendPoll request = new SendPoll("chatId", "Huh", "Bird", "Bear", "Chicken"); |
There was a problem hiding this comment.
this is the same, it's just another version of sendPoll test and should be in TelegramBotTest.
(it's probably already covered there)
|
|
||
| @Test | ||
| public void toParamValue() { | ||
| Assert.assertEquals("1",client.toParamValue(1)); |
There was a problem hiding this comment.
this is also covered by higher integration test.
it does look useful, and maybe if it will be a separate "Serializer" class then this test make sense.
but in this case I think we can keep toParamValue as private
| return new ReplyKeyboardMarkup().addRow(firstLine); | ||
| } | ||
|
|
||
| public static ReplyKeyboardMarkup create(KeyboardButton... firstLine) { |
There was a problem hiding this comment.
can we also make sure that all new constructors are used in TelegramBotTest and thus covered?
you can just change some existing to new ones.
|
the biggest issue that you combined all these changes in 1 PR. there is also 1 test fail: anyway now it's reviewed and we are closer to merge, hope with next iteration we have all tests green :) |
* Keyboard improvements + generics * Applied review comments
|
thank you @marcovaneck! |
Hi,
I renamed the keyboard-methods to addRow, and only the trivial-changes. The changes to TelegamBotClient have been excluded. They gave me more information on the incorrect-message it sent (deserialization & generics are not always friends).
Best regards