Skip to content

Add Telegram Bot message reactions #146354

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

Merged

Conversation

aviadlevy
Copy link
Contributor

@aviadlevy aviadlevy commented Jun 8, 2025

Breaking change

Proposed change

Add message reaction to Telegram bot

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @aviadlevy

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

home-assistant bot commented Jun 8, 2025

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@aviadlevy aviadlevy force-pushed the telegram_bot_set_message_reaction branch from b299506 to faae606 Compare June 9, 2025 06:12
@aviadlevy aviadlevy requested a review from hanwg June 9, 2025 06:16
Copy link
Contributor

@hanwg hanwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this and it works fine.

@aviadlevy aviadlevy force-pushed the telegram_bot_set_message_reaction branch 2 times, most recently from 38d9dca to 415aa6f Compare June 11, 2025 07:25
@hanwg
Copy link
Contributor

hanwg commented Jun 11, 2025

@aviadlevy for the merge conflict, I think you only need to move your test to the end of the test_telegram_bot.py file.

@aviadlevy aviadlevy force-pushed the telegram_bot_set_message_reaction branch from 415aa6f to 8f9cee1 Compare June 11, 2025 14:24
@aviadlevy
Copy link
Contributor Author

@aviadlevy for the merge conflict, I think you only need to move your test to the end of the test_telegram_bot.py file.

I just rebased a few hours ago due to conflicts.
At least every rebase brings this MR one step closer to review and merge. 🤞

@aviadlevy aviadlevy force-pushed the telegram_bot_set_message_reaction branch from 8f9cee1 to ec2ef5e Compare June 11, 2025 18:18
Copy link
Member

@abmantis abmantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just 2 minor changes, but otherwise LGTM

@home-assistant home-assistant bot marked this pull request as draft June 12, 2025 16:21
@abmantis
Copy link
Member

Oh by the way, please avoid rebasing after getting a review and use merge instead. It makes it easier to review changes since Github does not deal with git history changes well for partial reviews.

@aviadlevy
Copy link
Contributor Author

Oh by the way, please avoid rebasing after getting a review and use merge instead. It makes it easier to review changes since Github does not deal with git history changes well for partial reviews.

Thanks for the review!
Just to confirm - does that mean I should avoid amending commits too, and instead push every change as a new commit?

@aviadlevy aviadlevy marked this pull request as ready for review June 12, 2025 16:42
@home-assistant home-assistant bot requested review from hanwg and abmantis June 12, 2025 16:42
@abmantis
Copy link
Member

Oh by the way, please avoid rebasing after getting a review and use merge instead. It makes it easier to review changes since Github does not deal with git history changes well for partial reviews.

Thanks for the review! Just to confirm - does that mean I should avoid amending commits too, and instead push every change as a new commit?

Yes. All commits will be squashed when merging to dev, so it is fine to have multiple commits in the PR.

@aviadlevy aviadlevy requested a review from abmantis June 14, 2025 20:14
Copy link
Member

@abmantis abmantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@home-assistant home-assistant bot marked this pull request as draft June 15, 2025 02:09
@aviadlevy aviadlevy marked this pull request as ready for review June 15, 2025 05:56
@home-assistant home-assistant bot requested a review from abmantis June 15, 2025 05:56
Copy link
Member

@abmantis abmantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aviadlevy !

By the way, for future PRs I would suggest avoiding adding unrelated changes (like updating other tests) in the same PR, as noted in https://developers.home-assistant.io/docs/review-process#creating-the-perfect-pr

@abmantis abmantis merged commit cce8782 into home-assistant:dev Jun 16, 2025
34 checks passed
@aviadlevy aviadlevy deleted the telegram_bot_set_message_reaction branch June 16, 2025 13:49
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants