Skip to content

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