Skip to content

feat: integrate pyromod patches #1

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.

Sign up for GitHub

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

Jump to bottom
Merged
alissonlauffer merged 10 commits into from
Jan 16, 2024
Merged

feat: integrate pyromod patches #1

alissonlauffer merged 10 commits into from
Jan 16, 2024

Conversation

@alissonlauffer
Copy link
Member

@alissonlauffer alissonlauffer commented Dec 2, 2023 •
edited
Loading

Description

This PR integrates Pyromod patches into Hydrogram. The code has been reformatted and lined according to the project's style guidelines.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes

alissonlauffer
alissonlauffer commented Dec 2, 2023
View reviewed changes
hydrogram/client.py Outdated
inline_message_id: Optional[Union[str, List[str]]] = None,
):
"""
Creates a listener and waits for it to be fulfilled.
Copy link
Member Author

@alissonlauffer alissonlauffer Dec 2, 2023

Choose a reason for hiding this comment

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

Documentation should be rewritten to follow Hydrogram's style.

@alissonlauffer alissonlauffer force-pushed the pyromod branch 2 times, most recently from 4d447b0 to 5752af6 Compare December 2, 2023 18:19
HitaloM
HitaloM requested changes Dec 3, 2023
View reviewed changes
Copy link
Member

@HitaloM HitaloM left a comment

Choose a reason for hiding this comment

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

Everything looks good, but the docstrings should follow the Hydrogram format so as not to have dissonance in the docs, and we should also add the pyromod functions to the documentation with examples.

@alissonlauffer alissonlauffer changed the base branch from main to dev December 4, 2023 20:37
@HitaloM HitaloM added the enhancement New feature, request or code enhancement. label Dec 4, 2023
@alissonlauffer
Copy link
Member Author

alissonlauffer commented Jan 6, 2024

I think this should be done now. Just one thing: Message.wait_for_click (which exists in pyromod) has a from_user_id argument, and it doesn't fallback to Message.from_user.id if left unspecified (it should, since we're dealing with the context of a message, and not a chat). I suggest API changes in this direction, but that's up to Cezar.

The style check currently fails because it is being tested against a new version of Ruff, which should be updated at our runtime in a different PR.

HitaloM
HitaloM approved these changes Jan 6, 2024
View reviewed changes
Copy link
Member

@HitaloM HitaloM left a comment

Choose a reason for hiding this comment

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

Everything looks good!

@usernein
Copy link
Member

usernein commented Jan 10, 2024

has a from_user_id argument, and it doesn't fallback to Message.from_user.id if left unspecified (it should, since we're dealing with the context of a message, and not a chat)

@alissonlauffer in my opinion, it makes no sense to fallback to Message.from_user.id

Message.wait_for_click means you are waiting for someone to click an inline button on this message and pyromod will give you the corresponding CallbackQuery object.

The from_user_id parameter is used to specify that you only want to receive the click if it comes from a specific user. If it falls back to Message.from_user.id, this means that you will only accept a click if it comes from the bot itself (not a userbot) that sent the message with the inline button, which makes no sense.

If from_user_id is None, which is the actual default value, it will accept clicks from anyone. If you want to specify it to be someone, you have to explicitly specify a user or a list of users (with their ids or usernames).

I don't think it's a good choice to implicitly limit the allowed users to be only the bot that sent the keyboard if the user didn't specify anyone in from_user_id.

usernein
usernein approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@usernein usernein left a comment

Choose a reason for hiding this comment

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

LGTM

@alissonlauffer
chore(pyromod): re-add user_id in `Message.{listen,ask,stop_listening…
c67d0d3 
…}` methods and cleanup chat_id and user_id typing hints
HitaloM
HitaloM approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@HitaloM HitaloM left a comment

Choose a reason for hiding this comment

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

Very good, thank you my friend

@alissonlauffer alissonlauffer merged commit 855e406 into dev Jan 16, 2024
@alissonlauffer alissonlauffer deleted the pyromod branch January 16, 2024 03:48
HitaloM added a commit that referenced this pull request Feb 2, 2024
@HitaloM
refactor: changed copyright holder of pyromod
ec53370 
The PR of Pyromod implementation (#1) was submitted before the change in organization and copyright holder. As a result, we forgot to update the holder information in the files from Amano LLC to Hydrogram.
HitaloM added a commit that referenced this pull request May 22, 2024
@HitaloM
docs: improvements and fixes (#19)
4541307 
* docs: fixed typos in the contributing guide

* fix: tgcrypto link in speedups doc

* refactor: improved sphinx configuration

* refactor: changed copyright holder of pyromod

The PR of Pyromod implementation (#1) was submitted before the change in organization and copyright holder. As a result, we forgot to update the holder information in the files from Amano LLC to Hydrogram.

* docs: use correct category for news fragment 15

* Merge branch 'dev' of https://github.com/hydrogram/hydrogram into docs-improvements

* chore: update furo to 2024.1.29

* docs: fixed a typo in github url

* docs: disable `napoleon_preprocess_types`

It was causing problems with the formatting of the types in the documentation pages

* docs(contributing): improved feedback and commits section

* Merge branch 'dev' of https://github.com/hydrogram/hydrogram into docs-improvements

* fix(news): fixed a typo

* chore: update sphinx-autobuild

* docs: added mtproto-vs-bot-api image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@usernein usernein usernein approved these changes

@HitaloM HitaloM HitaloM approved these changes

Assignees

No one assigned

Labels

enhancement New feature, request or code enhancement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alissonlauffer @usernein @HitaloM