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

Add FREEZE and THAW methods for serialisation #520

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

shawnlaffan
Copy link
Contributor

@shawnlaffan shawnlaffan commented Jan 11, 2025

Tests work with Serial, JSON::MaybeXS, CBOR::XS and CBOR::Free. (Edit: CBOR::Free was a false positive - it does not support FREEZE/THAW).

I also had a look at testing with some of the YAML modules but they do not support FREEZE/THAW callbacks.

Could probably be squashed into a single commit before merging, thus cleaning up debugging cruft.

Documentation is still needed. It should just comprise a note that serialisers that conform to Types::Serialiser are (should be) supported.

I'm not sure where to add it, though, as it does not fit under PDL::IO::Storable. Perhaps its contents could be shifted to a new PDL::IO::Serialisation or similar which is then loaded by default with the rest of PDL. That might be too large a change, though.

@coveralls
Copy link

coveralls commented Jan 11, 2025

Coverage Status

coverage: 67.071% (+0.7%) from 66.357%
when pulling e60317a on shawnlaffan:freeze_thaw
into 0299134 on PDLPorters:master.

@mohawk2
Copy link
Member

mohawk2 commented Jan 11, 2025

I feel like PDL::IO::Storable is a good place - it's just a vehicle to add PDL methods, which is what's needed here? Why do you say it doesn't fit?

Thank you for your great work on this! Could I ask you to add the doc notes you mention?

Don't worry about the commits, I'll do a squash. Could I also ask you though to put the tests in a section in t/storable.t instead of a separate file?

@shawnlaffan
Copy link
Contributor Author

The reason for changing things is simply the general vs the specific. Storable is a specific serialisation method.

@mohawk2
Copy link
Member

mohawk2 commented Jan 12, 2025

This is great - thank you!

@mohawk2
Copy link
Member

mohawk2 commented Jan 13, 2025

I've rebased this, and had to make a couple of tweaks (see my commit changes - JSON::PP doesn't work). As soon as the CI blesses it, I'll merge it.

@mohawk2 mohawk2 merged commit 64bae89 into PDLPorters:master Jan 13, 2025
81 checks passed
@mohawk2
Copy link
Member

mohawk2 commented Jan 13, 2025

Thanks again! It looks like your repo has a number of branches you may not need anymore. Time for a spring clean? :-)

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