Skip to content

Clear up import policy #2671

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
merged 9 commits into from
Sep 22, 2021
Merged

Clear up import policy #2671

merged 9 commits into from
Sep 22, 2021

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Sep 19, 2021 •
edited
Loading

closes #2468

Some notes:

  • I know 120 files sounds bad the the majority of changes is really just imports :D
  • ext.utils.promise.Promise was deliberately not moved to ext.promise.Promise because since the removal of MessageQueue users should almost never have contact with that class. You can view that at RTD
  • While at it I split up that messy tg.utils.helpers module into a number of separate modules and moved parts of the contents just to the place where it's actually used. Also added a bunch of additional tests
  • Added a new section in the docs for the modules in telegram that are not strictly API related

TODO:

  • check if we can add some more versionchanged directives
  • Wait for Warnings overhaul #2662 to me merged so that we can include the changes to the warnings
  • There's now tg.helpers and tg.utils.helpers maybe we can find a better name for one of the two to avoid confusion …

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) -- This can probably still be improved

@Bibo-Joshi Bibo-Joshi added 🛠 refactor change type: refactor 🛠 breaking change type: breaking labels Sep 19, 2021
@Bibo-Joshi Bibo-Joshi added this to the v14 milestone Sep 19, 2021
@Bibo-Joshi Bibo-Joshi requested a review from JosXa September 19, 2021 17:07
github-actions[bot]
github-actions bot reviewed Sep 19, 2021
View reviewed changes
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 requested review from harshil21 and starry-shivam and removed request for JosXa September 20, 2021 16:48
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review September 20, 2021 16:48
harshil21
harshil21 requested changes Sep 21, 2021
View reviewed changes
Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Good changes. Some things we can still change:

  • While at it, we can also rename base.py to telegramobject.py, cause for some reason in rtd the 'bases' section doesn't link is broken (just updated my sphinx to 4.2.0 from 4.0.2 and now this is fixed!), and I suppose telegramobject.py is more explicit.

  • Also telegram.ext.filters Module is listed under the 'Handlers' section. Maybe move it up where the rest of telegram.ext is (i.e with Updater and the rest)?

  • I noticed telegramobject.rst doesn't have :github_url:, maybe add that?

@Bibo-Joshi
Copy link
Member Author

  • While at it, we can also rename base.py to telegramobject.py, cause for some reason in rtd the 'bases' section doesn't link is broken (just updated my sphinx to 4.2.0 from 4.0.2 and now this is fixed!), and I suppose telegramobject.py is more explicit.

The broken link is not really related to the file name, but I renamed anyway. see sphinx-doc/sphinx#9026
But this is actually a good reminder to follow up on sphinx-doc/sphinx#4944 being fixed now 👍 we should do that in the doc-fixes branch though

  • Also telegram.ext.filters Module is listed under the 'Handlers' section. Maybe move it up where the rest of telegram.ext is (i.e with Updater and the rest)?

I moved it there on purpose a while back, because filters basically belongs to MessageHandler … at least for me that makes more sense …

  • I noticed telegramobject.rst doesn't have :github_url:, maybe add that?

✔️

harshil21
harshil21 reviewed Sep 22, 2021
View reviewed changes
@Bibo-Joshi @harshil21
fix typo
5239635 
Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
@Bibo-Joshi Bibo-Joshi merged commit 4f21c06 into v14 Sep 22, 2021
@Bibo-Joshi Bibo-Joshi deleted the clear-up-imports branch September 22, 2021 14:49
@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.
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@harshil21 harshil21 harshil21 approved these changes

@starry-shivam starry-shivam Awaiting requested review from starry-shivam

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

Successfully merging this pull request may close these issues.
2 participants
@Bibo-Joshi @harshil21