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

Rework CAtom_setstate #234

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

frmdstryr
Copy link
Contributor

Any suggestions for improvements are welcome:

  1. PyMapping_Check is kind of broken so tuples and lists pass it
  2. As mentioned in the other PR setattr triggers notifications
  3. I don't like how it modifies the passed in state by deleting the frozen flag but checking every key makes it a lot slower (~30%)

Copy link

codecov bot commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.67%. Comparing base (54adc3f) to head (cf0d164).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #234   +/-   ##
=======================================
  Coverage   97.67%   97.67%           
=======================================
  Files          24       24           
  Lines        1074     1074           
  Branches      162      162           
=======================================
  Hits         1049     1049           
  Misses         12       12           
  Partials       13       13           

@MatthieuDartiailh
Copy link
Member

The transition to c++ was done in https://github.com/nucleic/atom/pull/182/files#diff-e9ee00a921e9e460fd604a1d6fa1541ee97e2767aac2f335dd1448567d9d6ebfL648-L658 and we simply copied what existed on the Python side. When unpickling an object (I do not consider other possible abuse of __setstate__) the only observer that may be called are static observers. We could change this but it may be a breaking change.

@sccolbert
Copy link
Member

I think I may have been lazy when implementing get/set state back in the day, just to get it out the door.

I think it's reasonable to assume that a call to __setstate__ would not trigger observers, since it's primary use is de-pickling.

But as you say, that might be a breaking change. Another thing that I would do very differently if afforded the opportunity.

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

LGTM but can you investigate the Python 3.13 failure

@frmdstryr frmdstryr force-pushed the redo-catom-setstate branch from 83e27cb to cf0d164 Compare January 21, 2025 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants