Skip to content

Remove BP.insert/replace_bot #2893

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

Merged
merged 22 commits into from
Mar 12, 2022
Merged

Remove BP.insert/replace_bot #2893

merged 22 commits into from
Mar 12, 2022

Conversation

harshil21
Copy link
Member

@harshil21 harshil21 commented Feb 10, 2022

Closes #2882

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)
  • Created new or adapted existing unit tests
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs

Breaking changes:

  • insert/replace_bot was removed so user may have to use set_bot(bot) before using shortcuts.

@harshil21 harshil21 added enhancement 🛠 breaking change type: breaking labels Feb 10, 2022
@harshil21 harshil21 added this to the v14 milestone Feb 10, 2022
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.

Looks good overall IMO :) Was able to go right into nitpicking ;P

@harshil21 harshil21 marked this pull request as ready for review February 15, 2022 22:41
@harshil21 harshil21 requested a review from Bibo-Joshi February 15, 2022 22:41
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 the work on this! I left a number of comments.

Additionally, please elaborate in the docstring of Bot why bots should not be serialized and that for that reason pickling will raise an exception

@harshil21 harshil21 added the 📋 do-not-merge-yet work status: do-not-merge-yet label Feb 20, 2022
@harshil21 harshil21 mentioned this pull request Feb 21, 2022
12 tasks
@harshil21
Copy link
Member Author

Current approach of __setstate__ and __deepcopy__ fails when attribute names of an object ≠ the expected argument names (e.g. a class taking attr as argument, but the self variable is assigned as: self._attr = attr).

Also fails when there are extra args and the constructor does not accept **kwargs.
So will redesign this.

@harshil21
Copy link
Member Author

harshil21 commented Mar 7, 2022

Another way to achieve deepcopying without actually defining __deepcopy__, would be to use was_called_by in __getstate__ as follows:

import copy
from telegram.ext._utils.stack import was_called_by 
...
if was_called_by(inspect.currentframe(), Path(copy.__file__)):
    return self._get_attrs(include_private=True, recursive=False)
return self._get_attrs(include_private=True, recursive=False, remove_bot=True)

but didn't use this since it feels a bit hacky and unpythonic, although its elegant.

@harshil21 harshil21 requested a review from Bibo-Joshi March 7, 2022 19:40
@harshil21 harshil21 added 📋 pending-review work status: pending-review and removed 📋 do-not-merge-yet work status: do-not-merge-yet labels Mar 7, 2022
return self._bot
if pid == _REPLACED_UNKNOWN_BOT:
return None
raise pickle.UnpicklingError("Found unknown persistent id when unpickling!")
Copy link
Member Author

Choose a reason for hiding this comment

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

codecov gonna complain about this since I don't think it's possible to test this...

@harshil21 harshil21 removed the 📋 pending-review work status: pending-review label Mar 8, 2022
@harshil21 harshil21 added the 📋 pending-merge work status: pending-merge label Mar 11, 2022
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.

Finally get's a LGTM from me 😃 Thanks for the effort you put into this! Is there anything left from your side? Otherwise we can merge.

Copy link
Member Author

@Bibo-Joshi Nope, all done and am happy with it now

@Bibo-Joshi Bibo-Joshi merged commit d117d21 into v14 Mar 12, 2022
@Bibo-Joshi Bibo-Joshi deleted the persistence-refactor branch March 12, 2022 11:27
@harshil21 harshil21 removed the 📋 pending-merge work status: pending-merge label Mar 12, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2022
@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.
Labels
🛠 breaking change type: breaking 🔌 enhancement pr description: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants