Skip to content

Further clear up imports policy: Private modules #2680

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
Closed
Bibo-Joshi opened this issue Sep 30, 2021 · 5 comments
Closed

Further clear up imports policy: Private modules #2680

Bibo-Joshi opened this issue Sep 30, 2021 · 5 comments
Assignees
@kencx
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Milestone
v20.0a0

Comments

@Bibo-Joshi
Copy link
Member

Is your feature request related to a problem? Please describe.

Currently all the modules like telegram.{message, chat, user, …} look like they are public. However, we don't consider them to be part of the public API and they should not be treated as such by the user. The only thing that we guarantee to work is from telegram import Message, Chat, User, … and not from telegram.message import Message.

Describe the solution you'd like

Rename all the private modules with a leading underscore, i.e. all files that contain classes that are imported in the __init__.
This is in accordance with PEP8:

Even with __all__ set appropriately, internal interfaces (packages, modules, classes, functions, attributes or other names) should still be prefixed with a single leading underscore.

this should also be done for the telegram.ext package except for the module telegram.ext.filters. The packages telegram(.ext).utils should also be renamed to telegram(.ext)._utils and dropped from the documentation - unless this leads to pre-commit complaining about importing private packages. If it does, we'll have to re-evaluate.

Additional context
@Bibo-Joshi Bibo-Joshi added hacktoberfest 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Sep 30, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Sep 30, 2021
@Bibo-Joshi Bibo-Joshi mentioned this issue Sep 30, 2021
Closed
@kencx
Copy link
Contributor

kencx commented Oct 1, 2021

Hi, I would like to work on this!

@Bibo-Joshi
Copy link
Member Author

Nice :) I assigned you.

@kencx
Copy link
Contributor

kencx commented Oct 1, 2021

mypy's type checking fails when some modules are made private with a leading underscore.

The modules affected are: chat, message, user, callbackquery, inline.inlinequery, passport.credentials, passport.passportdata, passport.passportfile, payment.precheckoutquery, payment.shippingquery. All other modules pass all unit and pre-commit tests.

For example, in a branch where only message.py is renamed to _message.py (limited to first 10 errors):

image

I have not identified the common cause yet as its my first time working with mypy.

Hope someone can point me in the right direction, thanks! :)

@Bibo-Joshi
Copy link
Member Author

Hi. This is in deed a bit tricky, so no worries :D {Message, Chat, …}.bot are usually None if you manually instantiate those classes, but because that's seldomly the case, we told mypy to just ignore those errors. That's this part of setup.cfg:

python-telegram-bot/setup.cfg

Lines 61 to 64 in c3e3bb7

# Disable strict optional for telegram objects with class methods
# We don't want to clutter the code with 'if self.bot is None: raise RuntimeError()'
[mypy-telegram.callbackquery,telegram.chat,telegram.message,telegram.user,telegram.files.*,telegram.inline.inlinequery,telegram.payment.precheckoutquery,telegram.payment.shippingquery,telegram.passport.passportdata,telegram.passport.credentials,telegram.passport.passportfile,telegram.ext.filters]
strict_optional = False

If you change the names to leading underscores here, too, it should work. If there are any mypy errors left that you can't resolve, you can just PR and we'll have a look together :)

BTW: We indeed aim to resolve this mypy-workaround with #2505

@kencx kencx mentioned this issue Oct 1, 2021
Merged
4 tasks
@kencx
Copy link
Contributor

kencx commented Oct 1, 2021

Thanks that was indeed the issue. Its fixed now and I just opened a PR :)

@github-actions github-actions bot locked and limited conversation to collaborators Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@kencx kencx

Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Milestone
v20.0a0
Development

No branches or pull requests
2 participants
@Bibo-Joshi @kencx