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

Introduce atrium_api::types::Unknown for unknown fields #205

Closed
wants to merge 1 commit into from

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Jul 27, 2024

Closes #204.

@str4d str4d force-pushed the 204-unknown-field-type branch from 2b17475 to 018fde6 Compare July 27, 2024 23:32
@str4d str4d force-pushed the 204-unknown-field-type branch from 018fde6 to e1d4b5c Compare July 27, 2024 23:44
let typ = match name {
NamedUnknown::Field("didDoc") => quote!(crate::did_doc::DidDocument),
NamedUnknown::Field("record") | NamedUnknown::Array("relatedRecords") => {
quote!(crate::records::Record)
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 figured that unknown fields named record or relatedRecords should still use Record, so after changing the default to Unknown I added these two extra cases, which affected the following lexicons: https://github.com/sugyan/atrium/compare/2b1747594b9e1fd711662416971c28c3cd7e1843..e1d4b5ca1e4c3ab9e891086bd4e2200c549ff742

@@ -46,7 +46,7 @@ pub struct ViewRecordData {
pub repost_count: Option<i64>,
pub uri: String,
///The record data itself.
pub value: crate::records::Record,
pub value: crate::types::Unknown,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, the moderation_quoteposts test now fails to compile because it expects a known record type here. Scanning over the other changes, it does seem that currently an unknown field named value is consistently a Record, but then why do the lexicons not use an open union?

I think we're going to need to ask for further guidance here. It might be that what we need to do, instead of my current changes, is to make UnknownData.type an Option<String> and then enforce that it is Some(_) everywhere that it is used for a union field.

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've commented about the issue here: bluesky-social/atproto#999 (comment)

@sugyan
Copy link
Owner

sugyan commented Aug 12, 2024

Resolved #204 by #209.

@sugyan sugyan closed this Aug 12, 2024
@str4d str4d deleted the 204-unknown-field-type branch August 12, 2024 16:43
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.

Record can't parse (at least some) unknown types
2 participants