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

Catch exceptions raised while copying __dict__/__slots__ in BasePersi… #2564

Merged
merged 11 commits into from
Jun 20, 2021
Merged

Catch exceptions raised while copying __dict__/__slots__ in BasePersi… #2564

merged 11 commits into from
Jun 20, 2021

Conversation

zeroone2numeral2
Copy link
Contributor

@zeroone2numeral2 zeroone2numeral2 commented Jun 17, 2021

…stence.replace/insert_bot()

Also updated the docstrings to reflect the changes in behavior with unexpected errors

closes #2563

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)

…stence.replace/insert_bot()

Also updated the docstrings to reflect the changes in behavior with unexpected errors
@zeroone2numeral2
Copy link
Contributor Author

I was looking at the contribution guidelines. Quiting:

Add .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the associated documentation of your changes, depending on what kind of change you made. This only applies if the change you made is visible to an end user. The directives should be added to class/method descriptions if their general behaviour changed and to the description of all arguments & attributes that changed.

I did made some changes to the docstring of the replace_bot and insert_bot methods, so I built the docs and looked for their docs source. But I can't find any versionchanged string. I'm not sure if I'm supposed to do something or not

@zeroone2numeral2
Copy link
Contributor Author

zeroone2numeral2 commented Jun 17, 2021

Question about tests: I've added a uuid.UUID object to CustomClass, which is immutable and would raise a TypeError in insert/replace_bot. I tried to run the tests as described in the contributions guidelines with pytest -v, but I'm a bit lost: the test for test_persistence.py failed with the following output: https://nekobin.com/loqazeruxi

I'm not entirely sure how I should update the test

Edit: it fails even after 9e67463

@Bibo-Joshi Bibo-Joshi self-requested a review June 17, 2021 14:10
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 PR!

Regarding docstrings: Those look fine, shouldn't be a problem to build them 👌
Regarding tests: I forgot that pytest treats warnings as errors if not handled otherwise … On a closer look I see that we have a for unpickable objects test here. You can copy that and replace lock with uuid (you can skip the CustomClass part and revert your current changes to the tests)

Also, pre-commit complains about a few things. Please make sure to run pre-commit install on your machine so that pre-commit can run on every commit. You can also have it run manually via pre-commit run. Have a look at the contribution guide for more details :)

telegram/ext/basepersistence.py Outdated Show resolved Hide resolved
@zeroone2numeral2 zeroone2numeral2 marked this pull request as ready for review June 18, 2021 07:54
@Bibo-Joshi
Copy link
Member

Except for the typo, this looks good to me :)

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>
@Bibo-Joshi Bibo-Joshi merged commit 105f1cc into python-telegram-bot:master Jun 20, 2021
@Bibo-Joshi
Copy link
Member

Thanks for the contribution @zeroone2numeral2 :)

@zeroone2numeral2
Copy link
Contributor Author

Thanks @harshil21 for the commit! I've been busy during the weekend

And thanks for the merge :)

@zeroone2numeral2 zeroone2numeral2 deleted the persistence-dict-slots-exceptions branch June 21, 2021 06:40
sakibguy added a commit to sakibguy/python-telegram-bot that referenced this pull request Jun 22, 2021
Better Exception-Handling for BasePersistence.replace/insert_bot (python-telegram-bot#2564)
@github-actions github-actions bot locked and limited conversation to collaborators Jun 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] 13.1+ persistence migration script raises an exception when converting UUID objects
3 participants