Skip to content

Improve Warning Categories & Stacklevels#3674

Merged
Bibo-Joshi merged 10 commits intomasterfrom
warnings-categories
Apr 27, 2023
Merged

Improve Warning Categories & Stacklevels#3674
Bibo-Joshi merged 10 commits intomasterfrom
warnings-categories

Conversation

@Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Apr 21, 2023 •
edited
Loading

  • Fixed a few warning categories that were missed in API 6.6.
  • Unifies warning handling in Bot and ExtBot, ensuring that the stacklevel is correct for both
  • adds missing tests & modifies existing ones

TODO:

  • Update all warning tests to also check the category
  • Update tests to double check stacklevel in ExtBot vs Bot

@Bibo-Joshi Bibo-Joshi self-assigned this Apr 21, 2023
@Bibo-Joshi Bibo-Joshi added enhancement ⚙️ tests affected functionality: tests labels Apr 21, 2023
harshil21
harshil21 reviewed Apr 21, 2023
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.

nice catches

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review April 22, 2023 21:13
lemontree210
lemontree210 requested changes Apr 23, 2023
View reviewed changes
Copy link
Member

@lemontree210 lemontree210 left a comment

Choose a reason for hiding this comment

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

I'm obviously out of my depth here, but one typo I did catch! :)

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 23, 2023

Just a related note: stackleveling will get way more fun soon :D https://github.com/python-telegram-bot/python-telegram-bot/projects/8#card-88970053

@Bibo-Joshi
Review:
22a1883 
* Fix typo
* Make sure to only use the local information on stacklevel
harshil21
harshil21 reviewed Apr 24, 2023
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.

much more extensive testing, I like it

@Bibo-Joshi Bibo-Joshi changed the title Improve Warning Categories Improve Warning Categories & Stacklevels Apr 24, 2023
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 24, 2023

While working on #3673 I noticed that the subscription access to attributes of TelegramObject can mess with the stacklevel. Because message["text_markdown"] calls message.__getitem__("text_markdown") that's one more function call meaning that the warning will originate from _telegramobject.py instead of the user code. I guess the only workaround for that would be to catch & reraise all warnings in __getitem__ and that's not really clean … IMO the use case is small enough to ignore. Just wanted to note it in the context of this PR :)

@harshil21
Copy link
Member

harshil21 commented Apr 25, 2023

I wonder if there is a way to dynamically inspect the stacklevel and pass the correct value for cases like those...

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 25, 2023

@harshil21 I guess there is inspect.stack. I'm not so sure if I want to get the inspect module involved for this though ...

harshil21
harshil21 approved these changes Apr 26, 2023
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.

lgtm

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Apr 27, 2023

Test failures are unrelated - see #3685 and #3673. Merging.

@Bibo-Joshi Bibo-Joshi merged commit 3f444da into master Apr 27, 2023
@Bibo-Joshi Bibo-Joshi deleted the warnings-categories branch April 27, 2023 20:36
@Bibo-Joshi Bibo-Joshi mentioned this pull request Apr 27, 2023
Merged
15 tasks
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2023
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lemontree210 lemontree210 lemontree210 requested changes

@harshil21 harshil21 harshil21 approved these changes

Assignees

@Bibo-Joshi Bibo-Joshi

Labels

🔌 enhancement pr description: enhancement ⚙️ tests affected functionality: tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Bibo-Joshi @harshil21 @lemontree210