Skip to content

Move handler related files to _handlers subdirectory #4064

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
Bibo-Joshi merged 6 commits into from
Jan 15, 2024
Merged

Move handler related files to _handlers subdirectory #4064

Bibo-Joshi merged 6 commits into from
Jan 15, 2024

Conversation

@lucasmolinari
Copy link
Contributor

@lucasmolinari lucasmolinari commented Jan 11, 2024 •
edited by Bibo-Joshi
Loading

closes #4060.
Created a new directory telegram/ext/_handlers where all _*handler related files were moved, including filters.py.
Modified telegram/ext/__init__.py to point to the new directory.
Some files where pointing exactly to the handlers, for e.g telegram\ext\_application.py with the import statement from telegram.ext._basehandler import BaseHandler so I modified those too.

Bibo-Joshi
Bibo-Joshi reviewed Jan 11, 2024
View reviewed changes
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.

Is there a way to use from telegram.ext.filters import MessageFilter with the filters.py inside the _handlers directory? For now all from telegram.ext imports are working, except this one.

(#4060 (comment))

I tried around a bit and couldn't get it to work either. Also tried to find an explanation for the issue on the web, but didn't really get that far either. Sometimes, Python import behavior remains a mystery :D

Note that from telegram.ext import filters or import telegram.ext and then telegram.ext.filters will both still work. However, I wouldn't want to break anything here, so I propose that we just have the filters module at tg/ext/filters.py.

telegram/ext/__init__.py Outdated

from . import filters
from ._handlers import filters
from ._handlers._basehandler import BaseHandler
Copy link
Member

@Bibo-Joshi Bibo-Joshi Jan 11, 2024

Choose a reason for hiding this comment

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

Suggested change
from ._handlers._basehandler import BaseHandler
from ._handlers.basehandler import BaseHandler

Since the _handler module itself is marked as protected via the leading underscore, it would be fine with me to treat the individual files within as "public" content of that module.

@lucasmolinari
Copy link
Contributor Author

lucasmolinari commented Jan 12, 2024 •
edited
Loading

I tried around a bit and couldn't get it to work either. Also tried to find an explanation for the issue on the web, but didn't really get that far either. Sometimes, Python import behavior remains a mystery :D

I searched too, there is literally nothing about that in the web, at least I couldn't find it. Tried with AI too but I didn't got lucky :[

I did what you suggested and moved the filters module to tg/ext/filters.py and removed the underscore from the handler names in tg/ext/_handlers. Also, I noticed that some imports were still not working, so I fixed that with the help of pytest.

Since we are here, are these merge checks supposed to be succesful? Did I messed something up for that to not be working? At least the required ones seems to be fine.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 13, 2024

Since we are here, are these merge checks supposed to be succesful? Did I messed something up for that to not be working? At least the required ones seems to be fine.

The Bot API Tests is expected to fail until #4034 is merged. The other ones should not fail.
For the unit tests, I had a look at the failures and there seem to be 2 issuesd

  • in test_basehandler.py we use from telegram.ext._basehandler import BaseHandler. That should be changed to from telegram.ext import BaseHandler
  • in test_conversationhandler in the test test_unknown_state_warning, we check that some warnings are issued as expected and explicitly use the path to _conversationhandler.py. This should be updated as well.

DeepSource reports "Function with cyclomatic complexity higher than threshold" for ConversationHandler.check_update, however since you didn't actually touch that, I'm okay with ignoring that.

@lucasmolinari
Copy link
Contributor Author

lucasmolinari commented Jan 13, 2024

There it is, sorry about the commits that are only about formatting. First time working with pre-commit :)

@Poolitzer
Copy link
Member

Poolitzer commented Jan 13, 2024 •
edited
Loading

Only three commits from fighting pre-commit? What are you, some kind of wizard? I usually need at least five.

Don't worry we all started at some point, you get used to it. If you install it locally, git runs it before the actual commit, so you know it breaks before the commit goes through

@lucasmolinari
Copy link
Contributor Author

lucasmolinari commented Jan 13, 2024

Thank you both for been supportive. Learned a lot with this simple contribution, next time will be easier!

Bibo-Joshi
Bibo-Joshi approved these changes Jan 14, 2024
View reviewed changes
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.

LGTM :) Thanks for the work! @Poolitzer or maybe @clot27 would you mind giving it a once-over as well?

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

@clot27 clot27 left a comment

Choose a reason for hiding this comment

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

Nice work @lucasmolinari, LGTM!

@Bibo-Joshi Bibo-Joshi added the 🛠 code-quality change type: code-quality label Jan 15, 2024
@Bibo-Joshi Bibo-Joshi merged commit f452c13 into python-telegram-bot:master Jan 15, 2024
@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Jan 15, 2024

Thanks for the contribution!

@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Bibo-Joshi Bibo-Joshi Bibo-Joshi approved these changes

@clot27 clot27 clot27 approved these changes

Assignees

No one assigned

Labels

🛠 code-quality change type: code-quality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Move handler-related files to a new subdirectory

4 participants

@lucasmolinari @Bibo-Joshi @Poolitzer @clot27