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 TypedRecord and TypedRecordRef #1770

Merged
merged 22 commits into from Mar 24, 2023
Merged

add TypedRecord and TypedRecordRef #1770

merged 22 commits into from Mar 24, 2023

Conversation

bluejekyll
Copy link
Member

@djc, here are the types I was suggesting. It's possible that TypedRecordRef is not necessary if we deprecate Record and replace all use cases with TypedRecord. A different option instead of deprecating would be to do a type alias of type Record = TypedRecord<RData>. I'm not sure which would be better for downstream dependencies.

If you like this, I can add everything necessary to make all RecordData be compatible with this, currently only SOA implements RecordData.

crates/proto/src/rr/resource.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/record_data.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/rdata/soa.rs Outdated Show resolved Hide resolved
@bluejekyll
Copy link
Member Author

Ok, I'm really close to having this ready finally (after a long hiatus). There are a few places where things are now far simpler with this record type.

@bluejekyll bluejekyll force-pushed the new-typed-record branch 2 times, most recently from 174b64f to 8a510ac Compare January 2, 2023 01:30
@bluejekyll bluejekyll marked this pull request as ready for review January 2, 2023 04:22
@bluejekyll bluejekyll requested a review from djc January 2, 2023 04:22
@bluejekyll bluejekyll mentioned this pull request Jan 2, 2023
@bluejekyll
Copy link
Member Author

@djc, I'd love to land this before cutting the next release.

@djc
Copy link
Collaborator

djc commented Jan 5, 2023

I'll try to review this early next week.

@bluejekyll bluejekyll force-pushed the new-typed-record branch 2 times, most recently from f942742 to 3a3e6ae Compare February 7, 2023 02:04
@bluejekyll
Copy link
Member Author

Ok, @djc, this should be clean to land.

crates/proto/src/rr/record_data.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/record_data.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/record_data.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/mod.rs Show resolved Hide resolved
tests/integration-tests/tests/sqlite_authority_tests.rs Outdated Show resolved Hide resolved
crates/proto/src/rr/rdata/a.rs Outdated Show resolved Hide resolved
@bluejekyll bluejekyll merged commit 14c7424 into main Mar 24, 2023
@bluejekyll bluejekyll deleted the new-typed-record branch March 24, 2023 15:48
@bluejekyll
Copy link
Member Author

Thanks!

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.

None yet

2 participants