Skip to content

Conversation

Poolitzer
Copy link
Member

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs

If the PR contains API changes (otherwise, you can delete this passage)

  • New classes:

    • Added self._id_attrs and corresponding documentation
    • __init__ accepts **_kwargs
  • Added new shortcuts:

    • In Chat & User for all methods that accept chat/user_id
    • In Message for all methods that accept chat_id and message_id
    • For new Message shortcuts: Added quote argument if methods accepts reply_to_message_id
    • In CallbackQuery for all methods that accept either chat_id and message_id or inline_message_id
  • If relevant:

    • Added new constants at telegram.constants and shortcuts to them as class variables
    • Added new handlers for new update types
    • Added new filters for new message (sub)types
    • Added or updated documentation for the changed class(es) and/or method(s)
    • Updated the Bot API version number in all places: README.rst and README_RAW.rst (including the badge), as well as telegram.constants.BOT_API_VERSION
    • Added logic for arbitrary callback data in tg.ext.Bot for new methods that either accept a reply_markup in some form or have a return type that is/contains telegram.Message

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey there. Relax, I am just a little warning for the maintainers to release directly after merging your PR, otherwise we have broken examples and people might get confused :)

@Bibo-Joshi Bibo-Joshi added the ⚙️ examples affected functionality: examples label Mar 27, 2022
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Mar 27, 2022
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! changes themself look good except for the pre-commit errors. would you mind adding this section back to the pre-commit config?

- id: mypy
name: mypy-examples
files: ^examples/.*\.py$
args:
- --no-strict-optional
- --follow-imports=silent
additional_dependencies:
- certifi
- tornado>=6.1
- APScheduler==3.6.3
- cachetools==4.2.2
- . # this basically does `pip install -e .`

I had removed it temporarily while working on the asyncio changes …

PS: Don't know why codecov complains - no tracked files changed

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey! Looks like you edited the (dev) requirements or the pre-commit hooks. I'm just a friendly reminder to keep the pre-commit hook versions in sync with the dev requirements and the additional dependencies for the hooks in sync with the requirements :)

@Poolitzer
Copy link
Member Author

I added the config and fixed the black thing. Black itself has currently an import issue in the pre-commit and pylint complains locally, so I forced git to commit anyway and am interested in how it runs here.

@Poolitzer
Copy link
Member Author

I found the issue with black psf/black#2964, we either need to pin the click dependency to a lower version or upgrade the black version, what would you prefer and should I squeeze it in here or in a second PR?

Copy link
Member

@Poolitzer We're updating dependencies in #2925 so imo no need to do it here

PR #2925: Update Test Dependencies versions by harshil21

Copy link
Member

@harshil21 I'm okay with doing it here/in asyncio as the CI currently fails otherwise. #2925 is due only after asyncio

Copy link
Member Author

@Bibo-Joshi So to be less disruptive, I could hardpin the dependency, and we remove the hardpin and upgrade the version in #2925

Copy link
Member

@Poolitzer sounds okay. please add a corresponding note to #2925

@Poolitzer Poolitzer mentioned this pull request Mar 31, 2022
8 tasks
Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

The changes themself LGTM 👍 did you by any chance already test the examples? if not: do you want to before we merge or do we just do that later?

Copy link
Member Author

@Bibo-Joshi I didn't test it, I think that makes most sense before the first release.

Poolitzer and others added 2 commits March 31, 2022 19:24
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
@harshil21
Copy link
Member

Only thing I didn't run was passport, payment, and chatmemberbot. Rest all are working fine! Asyncio runs smooooth!

Resolved issues in examples/rawapibot.py via DeepSource Autofix
Co-authored-by: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com>
@Poolitzer
Copy link
Member Author

looks good to me

@Bibo-Joshi
Copy link
Member

Tested chatmemberbot. Runs fine except for a but in CMU.difference that I found (see #2947), but that's not due to the example.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Apr 10, 2022

Tested passport & payment example. For passports, I adjusted the html file a bit and found tdlib/telegram-bot-api#253 - which makes me want to stabilize Updater a bit more, but that's not for this PR.

If CI runs through & @harshil21 and @Poolitzer don't have anything to add, we can merge :)

@harshil21
Copy link
Member

All good from my side

@Poolitzer
Copy link
Member Author

Looks good to me as well

@Bibo-Joshi Bibo-Joshi merged commit 62fc4ff into asyncio Apr 10, 2022
@Bibo-Joshi Bibo-Joshi deleted the asyncio-examples branch April 10, 2022 13:54
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
⚙️ examples affected functionality: examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants