Skip to content

Clear up imports policy #2468

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

Closed
Bibo-Joshi opened this issue Apr 13, 2021 · 13 comments
Closed

Clear up imports policy #2468

Bibo-Joshi opened this issue Apr 13, 2021 · 13 comments
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Milestone

Comments

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Apr 13, 2021

We're currently not very consistent regarding what we make available directly via telegram.__init__ and what we don't. Some examples:

  • from telegram.error only TelegramError is available via telegram.__init__, but no of the other classes
  • from telegram.constants only some few constants are available.
  • TelegramDeprecationWarning is not available (see also Overhaul of warnings #2467)
  • the helper methods from utils.helper that are actually there to be used by users and not exclusively by PTB itself (e.g. escape_markdown, mention_user_*) are not available

IMO we should try to make a clear policy for this. IMHO it should be clear that all API-classes are directly available through telgeram. All the things mentioned above are from … lets call them "auxiliary modules". For these I basically see two options:

  1. Make all of that available directly through telegram
  2. Make none of it available through telegram. In this case I'd vote to at least move the above mentioned helper methods directly to telegram.utils and spare the user the .helpers, especially as users are in general not to be concerned with the other contents of utils.

Case 2 would be breaking & in that case we should wait until v14.
In any case I would aim to document the import policy somewhere … probably just at the index of the docs.
I'm slightly in favor of 2. What do the other @python-telegram-bot/developers think?

@Bibo-Joshi Bibo-Joshi added the 🛠 refactor change type: refactor label Apr 13, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Apr 13, 2021
@Poolitzer
Copy link
Member

I don't like 1, because that would mean loosing all the tidiness we have right now, or at least the attempt for this. I would really like 2 though, we should make a new class for the helpers imo, and provide there all the methods we intend for users from the helpers :D

@Bibo-Joshi
Copy link
Member Author

With Noam also in favor of 2, I'll add the 💥 tag and postpone to v14

@Bibo-Joshi Bibo-Joshi added the 🛠 breaking change type: breaking label Apr 15, 2021
@Poolitzer
Copy link
Member

Random thought, maybe we can do like from python-telegram-bot import ***? This has two tremendous upsides: No namespace issues anymore with other libraries/user files. And three clear subcategories: python-telegram-bot.telegram for all pure API stuff, python-telegram-bot.ext for all ext stuff we did, and python-telegram-bot.utils for utils we provide.

@Bibo-Joshi
Copy link
Member Author

Interesting ideas! In fact "project name = name of the only package provided by the project" is strongly encouraged by PEP 423.
However, hyphens in package names give syntax error (you'd have to do smth like __import__('python-telgeram-bot') and while underscores would be possible, PEP 8 discourages using them.

If the goal ist just to have the telegram package renamed to something that's more unique to PTB, one thus could e.g. use python_telegram_bot or ptb. If the goal is to satisfy both PEP 8 and PEP 423, we should rename python-telegram-bot to something different (including GH organization & repo, RTD, homepage etc). maybe just ptb. Or awesomegram. TBH, I'm not so sure if any of the two solutions are worth the trouble we'd bring upon both ourself and the users. AFAIK the only other project providing a telegram package is telegram which doesn't even do anything and all other projects like pyTelegramBotApi, pyrogram, aiogram, botogram & telethon have more unique package names.

Regarding structure: In your proposed solution, would python-telegram-bot contain anything directly? Is having to type an additional .telegram worth the added clarity? basename.ext instead of (basename.)telegram.ext sounds interesting, but it only makes sense if we add a basename. Same for .utils - here we'd also still need telegram/ext.utils for things are exclusively used for the internals (e.g. type aliases, timestamp encodings & such).

In any case, even if we decide to continue discussion in this direction, I guess we can still first go ahead with the changes mentioned above. Renaming/ordering the packages can still be done an the end of v14 phase 1 or after phase 2.

@Poolitzer
Copy link
Member

Poolitzer commented Jul 21, 2021

Interesting ideas! In fact "project name = name of the only package provided by the project" is strongly encouraged by PEP 423.
However, hyphens in package names give syntax error (you'd have to do smth like import('python-telgeram-bot') and while underscores would be possible, PEP 8 discourages using them.

Urgh, yeah, no underscores then

If the goal ist just to have the telegram package renamed to something that's more unique to PTB, one thus could e.g. use python_telegram_bot or ptb. If the goal is to satisfy both PEP 8 and PEP 423, we should rename python-telegram-bot to something different (including GH organization & repo, RTD, homepage etc). maybe just ptb. Or awesomegram. TBH, I'm not so sure if any of the two solutions are worth the trouble we'd bring upon both ourself and the users.

Agree on the too much trouble with renaming anything but the import. I would stick with something close to python_telegram_bot, I like underscores

AFAIK the only other project providing a telegram package is telegram which doesn't even do anything

But that's exactly the issue imo. People see import telegram and think "well, I will do a pip install telegram", this works and than they complain in the group. Having an unique import name will make them think, Im sure of it.

Regarding structure: In your proposed solution, would python-telegram-bot contain anything directly? Is having to type an additional .telegram worth the added clarity?

The main import will only contain subdirectories/modules/however you want to call them. And I do think it is worth the clarity, yes.

basename.ext instead of (basename.)telegram.ext sounds interesting, but it only makes sense if we add a basename. Same for .utils - here we'd also still need telegram/ext.utils for things are exclusively used for the internals (e.g. type aliases, timestamp encodings & such).

Hm, the whole idea was adding a basename? Not sure I get the but part. And we can either add telegram/ext specific util modules, or maybe we do basename.utils.telegram and basename.utils.ext.

In any case, even if we decide to continue discussion in this direction, I guess we can still first go ahead with the changes mentioned above. Renaming/ordering the packages can still be done an the end of v14 phase 1 or after phase 2.

Sure.

@Bibo-Joshi
Copy link
Member Author

But that's exactly the issue imo. People see import telegram and think "well, I will do a pip install telegram", this works and than they complain in the group. Having an unique import name will make them think, Im sure of it.

you're right, that's a point I missed. I guess this is also the reason for this

Urgh, yeah, no underscores then

and next line

Agree on the too much trouble with renaming anything but the import. I would stick with something close to python_telegram_bot, I like underscores

Whaaaaat? :P I do see how python_telegram_bot is nicely close to the project name, but it's also long und underscores are probably harder to type than letters …

@Poolitzer
Copy link
Member

Uh, I made an upsi there. I wanted to suggest import ptb, which is really short and looks cool, but ptb exists on pypi, so no improvement there. Maybe we have to stick with a long solution? import pythontelegrambot.

Its long, but you only import once.

@Bibo-Joshi
Copy link
Member Author

Meh … We should probably get opinions from the others on this before we get lost in discussion possible names :D

Its long, but you only import once.

once per file, though

@harshil21
Copy link
Member

harshil21 commented Jul 21, 2021

We should be able to get the telegram PyPI package name. See PyPI Help and
abandoned projects. In this case, it seems that we may be able to contact the owner here and ask them to transfer it to us.

@Bibo-Joshi
Copy link
Member Author

Totally forgot that this is possible :D Question is, what would we do if we had access to that pypi project?

  • publish a copy of PTB as telegram? Is that transparent?
  • Rename the project from PTB to telegram? (This will lead to more name conflicts. E.g. https://github.com/telegram is already taken, a domain telegram.some-tld will also not be that easy to find, and trying to secure meaningful group/channel names on TG itself will be near impossible … )

Copy link
Member

@Bibo-Joshi Pretty sure harshil meant PTB, not telegram

@harshil21
Copy link
Member

Question is, what would we do if we had access to that pypi project?

Hmm, good points. How about this:

We add this requirement in requirements.txt:
telegram!=0.0.1

And iirc, ever since pip v20, it won't allow conflicting requirements to be installed. So users will get a warning and they'll then uninstall telegram (if installed)

Copy link
Member

@harshil21 Doesn't stop them from installing telegram and complaining it wont work, but maybe eliminates the namespace issue, jup.

@Bibo-Joshi Bibo-Joshi mentioned this issue Sep 1, 2021
6 tasks
@Bibo-Joshi Bibo-Joshi mentioned this issue Sep 19, 2021
4 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 breaking change type: breaking 🛠 refactor change type: refactor
Projects
None yet
Development

No branches or pull requests

3 participants