Skip to content
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

Improve handling of custom objects in BP.insert/replace_bot #2151

Merged
merged 7 commits into from Nov 14, 2020

Conversation

Bibo-Joshi
Copy link
Member

if an object can't be copied, they're probably something special and don't contain a Bot instance. Therefore, those will now just be skipped.

Note: I noticed the latter while testing the changes on a personal project, where I have locks in some objects that have a custom pickle behaviour but not a custom copy behaivour

Copy link
Member

tsnoam commented Oct 16, 2020

@Bibo-Joshi Didn't look at the code yet, but just from the PR description, I wonder if we should also warn on objects which can't be copied.
To make it clear: the idea that we don't hide errors, but rather expose them.

Another option is to mandate that all objects must adhere to a certain interface. (I tend to like that one better)

@Bibo-Joshi
Copy link
Member Author

There's another small bug, that should be fixed in this PR: https://t.me/pythontelegrambotgroup/396509

TL;DR, in insert_bot replace if obj == self.REPLACE_BOT with if isinstance(obj, str) and obj == self.REPLACED_BOT:

@Bibo-Joshi Bibo-Joshi added this to the 13.1 milestone Nov 9, 2020
@Bibo-Joshi Bibo-Joshi changed the title Handle unpickable objects in BP.insert/replace_bot Improve handling of custom objects in BP.insert/replace_bot Nov 9, 2020
@@ -128,12 +128,12 @@ def set_bot(self, bot: Bot) -> None:
self.bot = bot

@classmethod
def replace_bot(cls, obj: object) -> object:
def replace_bot(cls, obj: object) -> object: # pylint: disable=R0911
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def replace_bot(cls, obj: object) -> object: # pylint: disable=R0911
def replace_bot(cls, obj: object) -> object:

Copy link
Member Author

Choose a reason for hiding this comment

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

pylint complains about the methods having too many return statements, so moving that line to the except Exception will make the test fail …

new_obj = copy(obj)
try:
new_obj = copy(obj)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except Exception:
except Exception: # pylint: disable=R0911

telegram/ext/basepersistence.py Outdated Show resolved Hide resolved
new_obj = copy(obj)
try:
new_obj = copy(obj)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
except Exception:
except Exception: # pylint: disable=R0911

telegram/ext/basepersistence.py Outdated Show resolved Hide resolved
Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

Looks like what we talked about.

@Bibo-Joshi Bibo-Joshi merged commit 8d9bb26 into master Nov 14, 2020
@Bibo-Joshi Bibo-Joshi deleted the handle-unpickable-objects branch November 14, 2020 02:08
@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants