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 dedicated types for DID and handle Lexicon string formats #103

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Feb 18, 2024

No description provided.

@str4d
Copy link
Contributor Author

str4d commented Feb 18, 2024

I've opened this PR to get review on the general approach, as well as the types that were simpler to implement. I am also working locally on:

  • A Datetime type that wraps chrono::DateTime.
    • This one is annoying because Lexicon uses the intersection of RFC 3339 and ISO 8601, and chrono can't parse the latter).
  • An Nsid type that wraps a String.
  • An AtUri type that contains an AtIdentifier, an Nsid, and a RecordKey.
  • A Uri type that might wrap url::Url.
    • I'm not yet sure if this will be valid. The at-uri format explicitly says it is not compatible with the WHAT-WG URL specification, which is what the url crate implements. The uri format seems like it is intended to be more general, but it does say it can include AT URIs, so that probably means this won't work.

Copy link
Owner

@sugyan sugyan left a comment

Choose a reason for hiding this comment

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

I've read the documentation.
Lexicon requires the intersection of RFC 3339 and ISO 8601, and chrono supports RFC 3339 with parse_from_rfc3339, but just because it can parse it does not mean it conforms to ISO 8601. So, if we want to implement it correctly, we need to exclude the ones that are not correct as ISO 8601. Am I correct in my understanding that this is the case?

If this library is more permissive than the spec (though it would need documentation), then all input for RFC 3339 format should be fine if parse succeeds, and the output in to_rfc3339 should meet Lexicon's requirements. I suspect that would work well enough, but what do you think?

atrium-api/src/types/string.rs Outdated Show resolved Hide resolved
@str4d
Copy link
Contributor Author

str4d commented Feb 18, 2024

I've read the documentation. Lexicon requires the intersection of RFC 3339 and ISO 8601, and chrono supports RFC 3339 with parse_from_rfc3339, but just because it can parse it does not mean it conforms to ISO 8601. So, if we want to implement it correctly, we need to exclude the ones that are not correct as ISO 8601. Am I correct in my understanding that this is the case?

Correct, that is indeed the case for parsing. I think this is primarily things like not parsing the T and Z as case-insensitive, but I'm trying to figure out a nice way to handle them that isn't just double-parsing.

If this library is more permissive than the spec (though it would need documentation), then all input for RFC 3339 format should be fine if parse succeeds, and the output in to_rfc3339 should meet Lexicon's requirements. I suspect that would work well enough, but what do you think?

For serialization, I'll be using DateTime::.to_rfc3339_opts(SecondsFormat::Micros, true), which should generate a timestamp string that is ISO 8601 compliant.

@str4d str4d force-pushed the lexicon-did-handle branch from 7b86f55 to 6060a7c Compare February 18, 2024 19:02
@str4d str4d force-pushed the lexicon-did-handle branch from 6060a7c to f3d18f8 Compare February 18, 2024 19:03
@str4d
Copy link
Contributor Author

str4d commented Feb 18, 2024

Rebased on main to fix merge conflict with #102 and address review comment.

@sugyan
Copy link
Owner

sugyan commented Feb 19, 2024

Thank you very much.
I will merge this once, can you please proceed with another pull-request for DateTime, Uri, etc.?

@sugyan sugyan merged commit 3d9e1f5 into sugyan:main Feb 19, 2024
4 checks passed
@str4d str4d deleted the lexicon-did-handle branch February 19, 2024 00:42
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.

2 participants