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 Data polyfill for ruby 3.1 #352

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nevans
Copy link
Collaborator

@nevans nevans commented Nov 9, 2024

For new data structs, I'd prefer frozen by default and I don't want to support the entire Struct API. Data is almost exactly what I want, but it's not available until ruby 3.2.

So this adds a DataLite class that closely matches ruby 3.2's Data class, and it can be a drop-in replacement for Data. Net::IMAP::Data is an alias for Net::IMAP::DataLite, so when we remove our implementation, the constant will resolve to ruby's ::Data.

Ideally, we wouldn't define this on newer ruby versions at all, but that breaks the YAML serialization for our test fixtures.

I originally implemented this by hand, with my own tests and partially by looking at ruby's struct.c. But I later copied all of the tests (and some of the implementation) from the polyfill-data gem. I've updated the tests so that they use Data as it is resolved inside the Net::IMAP namespace. Copyright notices have been added to the appropriate files to satisfy the MIT license terms.


This is a prerequisite for the following PRs (as currently implemented):

@nevans
Copy link
Collaborator Author

nevans commented Nov 9, 2024

@shugo what do you think about adding this, temporarily. We'd remove it with v0.6.0, when we raise the minimum required ruby to 3.2.

I actually started with PORO classes, but then I needed to implement the same #==, #eql?, #hash, #deconstruct, #deconstruct_keys multiple times.. at which point I wound up with this!

Alternatively, I could just use Struct as we've been doing. But I'd rather not need to support Struct's larger API surface.

@nevans
Copy link
Collaborator Author

nevans commented Nov 9, 2024

@saturnflyer I'm tagging you too, just so you know that I've copied your tests with very little modification. I'd already started down the path of my own implementation and I kept some of that, but I copied a bunch of your implementation too. Honestly, I should probably amend the commit to add you as Co-authored-by. 🙂

@nevans nevans force-pushed the data-polyfill branch 3 times, most recently from 90d5b85 to 2a4312f Compare November 11, 2024 16:03
@nevans nevans force-pushed the data-polyfill branch 3 times, most recently from 8abf44c to aa279bd Compare November 25, 2024 16:56
For new data structs, I don't want to commit to supporting the entire
Struct API and I'd prefer frozen by default.  `Data` is exactly what I
want but it's not available until ruby 3.2.

So this adds a DataLite class that closely matches ruby 3.2's Data
class and can be a drop-in replacement for Data.  Net::IMAP::Data is an
alias for Net::IMAP::DataLite, so when we remove this implementation,
the constant will resolve to ruby's ::Data.  The most noticable
incompatibility is that member names must be valid local variable names.

Ideally, we wouldn't define this on newer ruby versions at all, but that
breaks the YAML serialization for our test fixtures.  So, on newer
versions of ruby, this class inherits from `Data` and only reimplements
the two methods that are needed for YAML serialization.  This serves the
additional purpose of ensuring that the same tests pass for both the
core `Data` class and our reimplementation.

Most of the test code and some of the implementation code has been
copied from the polyfill-data gem and updated so that they use "Data" as
it is resolved inside the "Net::IMAP" namespace.  Copyright notices have
been added to the appropriate files to satisfy the MIT license terms.

Co-authored-by: Jim Gay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant