-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Telegrambost springboot mapping starter #1493
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
base: dev
Are you sure you want to change the base?
Conversation
…agus#1482) Co-authored-by: Ruben Bermudez <[email protected]>
…1475) Co-authored-by: Ruben Bermudez <[email protected]>
Fixed brackets on javadoc to show properly Co-authored-by: Ruben Bermudez <[email protected]>
yvasyliev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the whole change is about Bot Command mapping, I would name the module and related classes more specifically (e.g. telegrambots-springboot-longpolling-command-starter)
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public OkHttpClient okHttpClient() { | ||
| return new OkHttpClient(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is OkHttpClient bean created but never used?
| } | ||
|
|
||
| @Bean | ||
| public SpringLongPollingBot springLongPollingBot(TelegramBotProperties properties, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add @ConditionalOnMissingBean for extension opportunity
| @Bean | ||
| @ConditionalOnMissingBean(TelegramBotsLongPollingApplication.class) | ||
| public TelegramBotsLongPollingApplication telegramBotsApplication() { | ||
| return new TelegramBotsLongPollingApplication(); | ||
| } | ||
|
|
||
| @Bean | ||
| @ConditionalOnMissingBean | ||
| public TelegramBotInitializer telegramBotInitializer(TelegramBotsLongPollingApplication telegramBotsApplication, | ||
| ObjectProvider<List<SpringLongPollingBot>> longPollingBots) { | ||
| return new TelegramBotInitializer(telegramBotsApplication, | ||
| longPollingBots.getIfAvailable(Collections::emptyList)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since telegrambots-springboot-longpolling-starter is transitively included into into telegrambost-springboot-mapping-starter, no need to define TelegramBotsLongPollingApplication & TelegramBotInitializer beans explicitly
| <dependency> | ||
| <groupId>org.springframework.boot</groupId> | ||
| <artifactId>spring-boot-starter</artifactId> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spring-boot-starter dependency is redundant in this module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Missing
valueattribute. - I believe the alias should be
commandsinstead ofcommand.
| BotRequestMapping annotation = AnnotationUtils.findAnnotation(method, BotRequestMapping.class); | ||
| if (annotation != null) { | ||
| for (String command : annotation.value()) { | ||
| handlers.put(command, new BotHandlerMethod(bean, method)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check if a method has exactly one parameter of Update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be a record
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unused/duplicated imports
| } | ||
|
|
||
| public void handleUpdate(Update update) { | ||
| if (update.hasMessage() && update.getMessage().hasText()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's Message.isCommand() method
| <version>8.2.0</version> | ||
| </parent> | ||
|
|
||
| <artifactId>telegrambost-springboot-mapping-starter</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: telegrambots
52a184e to
9203912
Compare
No description provided.