Skip to content

Add warnings #2464

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 5 commits into from
Apr 16, 2021
Merged

Add warnings #2464

merged 5 commits into from
Apr 16, 2021

Conversation

starry-shivam
Copy link
Member

No description provided.

@starry-shivam
Warn if number of workers < one
02f143a 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi reviewed Apr 13, 2021
View reviewed changes
@starry-shivam
improve warning message
95e2c20 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi reviewed Apr 13, 2021
View reviewed changes
@Bibo-Joshi Bibo-Joshi mentioned this pull request Apr 13, 2021
Closed
@Bibo-Joshi
Copy link
Member

BTW, #2466 gave me the idea that we could do a warning when a ConversationHandler handler returns a state that is unknown, b/c in that case the conversation will break down. Maybe this could fit into this PR - we'd be making it a "add warings" pr :D

Here IG that I'd also go with warnings.warn, b/c the user can very well make sure that they only return known states. This could be added in update_state.

@harshil21
Copy link
Member

do a warning when a ConversationHandler handler returns a state that is unknown, b/c in that case the conversation will break down.

+1, will be easier to debug CH!

@Bibo-Joshi
Copy link
Member

Aaaand another warning we could add: if in CH allow_reentry is True and the CH has nested conversations, those should also have allow_reentry=True. See https://t.me/pythontelegrambotgroup/459818. This also deserves a comment in the docs. This should be double checked though, if that actually works

@starry-shivam
Copy link
Member Author

BTW, #2466 gave me the idea that we could do a warning when a ConversationHandler handler returns a state that is unknown, b/c in that case the conversation will break down.

When returning state that is unknown to conversation handler, the conversation gets stucked as long as user cancels it manually (via fallback) or until timeout gets triggered, shouldn't we also close the conversation after warning like: "Conversation handler returned unknown state, closing the current conversation..." Instead of just warning

@Bibo-Joshi
Copy link
Member

When returning state that is unknown to conversation handler, the conversation gets stucked as long as user cancels it manually (via fallback) or until timeout gets triggered, shouldn't we also close the conversation after warning like: "Conversation handler returned unknown state, closing the current conversation..." Instead of just warning

Mh, I don't think that's a good idea. Firstly, there's a chance we might close falsely in some edge cases. Also I think that it's rather surprising behaviour for CH to close conversation on its own and makes debugging even harder. And finally it would be somewhat breaking…

@starry-shivam
Copy link
Member Author

When returning state that is unknown to conversation handler, the conversation gets stucked as long as user cancels it manually (via fallback) or until timeout gets triggered, shouldn't we also close the conversation after warning like: "Conversation handler returned unknown state, closing the current conversation..." Instead of just warning

Mh, I don't think that's a good idea. Firstly, there's a chance we might close falsely in some edge cases. Also I think that it's rather surprising behaviour for CH to close conversation on its own and makes debugging even harder. And finally it would be somewhat breaking…

Oh right, makes sense.

@starry-shivam
warn if converstation handler return unknown state
9a7118d 
Signed-off-by: starry69 <starry369126@outlook.com>
@starry-shivam starry-shivam changed the title Warn if number of workers are less than one Add warnings Apr 15, 2021
Bibo-Joshi
Bibo-Joshi reviewed Apr 15, 2021
View reviewed changes
telegram/ext/conversationhandler.py Outdated
@@ -592,6 +592,10 @@ def update_state(self, new_state: object, key: Tuple[int, ...]) -> None:
)

elif new_state is not None:
if new_state not in self.states:
warnings.warn(
"Handler returned state which is unknown to the ConversationHandler."
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea: If the hanlder has a name, we could include that in the message. Makes debugging easier :) And maybe including the unknown state is useful, too …

@starry-shivam
Show handler name and state aswell, in warning
2cc8f9c 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi reviewed Apr 16, 2021
View reviewed changes
telegram/ext/conversationhandler.py Outdated
@@ -594,7 +594,8 @@ def update_state(self, new_state: object, key: Tuple[int, ...]) -> None:
elif new_state is not None:
if new_state not in self.states:
warnings.warn(
"Handler returned state which is unknown to the ConversationHandler."
f"Handler returned state {new_state} which is unknown to the "
f"ConversationHandler {self.name if self.name is not None else ''}."
Copy link
Member

Choose a reason for hiding this comment

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

Just one very minor nitpick left: when the CH has no name, there will be a trailing whitespace. We can do better, even in warnings :D

After this, I promise I'll merge! 😉

@starry-shivam
minor nitpick
7c03bb0 
Signed-off-by: starry69 <starry369126@outlook.com>
Bibo-Joshi
Bibo-Joshi approved these changes Apr 16, 2021
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.

Thanks for your patience!

@Bibo-Joshi
Copy link
Member

Wen can ignore deepsource until I'm done with #2454. Merging

@Bibo-Joshi Bibo-Joshi merged commit b63877b into master Apr 16, 2021
@Bibo-Joshi Bibo-Joshi deleted the worker-warning branch April 16, 2021 17:02
@github-actions github-actions bot locked and limited conversation to collaborators Apr 17, 2021
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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