Skip to content

Rework CAtom_setstate#234

Merged
MatthieuDartiailh merged 2 commits intonucleic:mainfrom
frmdstryr:redo-catom-setstate
Jan 29, 2025
Merged

Rework CAtom_setstate#234
MatthieuDartiailh merged 2 commits intonucleic:mainfrom
frmdstryr:redo-catom-setstate

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%)

@codecov
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).
Report is 4 commits behind head on main.

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

// If the -f key is present freeze the object
bool frozen = PyMapping_HasKey(state, atom_flags);
#if PY_VERSION_HEX >= 0x030D0000
Copy link
Member

Choose a reason for hiding this comment

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

I checked the documentation of this function but I do not understand why we need to use it here. Since we do not have the CI failure history, can you explain @frmdstryr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's just a warning but pytest still makes it fail: See the log here https://github.com/nucleic/atom/actions/runs/12874904293/job/35895250664 .

The HasKeyWithError version makes more sense anyways.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. It really shows what a bad job PyMapping_Check does. I will merge.

@MatthieuDartiailh MatthieuDartiailh merged commit 0750392 into nucleic:main Jan 29, 2025
18 checks passed
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