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

feat: Add tid/record-key string format #155

Merged
merged 5 commits into from
Apr 18, 2024
Merged

feat: Add tid/record-key string format #155

merged 5 commits into from
Apr 18, 2024

Conversation

sugyan
Copy link
Owner

@sugyan sugyan commented Apr 12, 2024

Resolve #154.
ref: https://atproto.com/specs/record-key

The RecordKey was originally defined by @str4d in types.rs, but has been moved to string.rs as well as other string formats (including regular expression and test updates due to definition updates). I would like to see if there are any objections to this move.

@sugyan sugyan changed the title Add tid/record-key string format feat: Add tid/record-key string format Apr 12, 2024
@sugyan sugyan marked this pull request as ready for review April 13, 2024 13:46
@sugyan
Copy link
Owner Author

sugyan commented Apr 13, 2024

@str4d could you review this?

Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

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

utACK'. My intention with the atrium_api::types::string module was for it to contain types in the String Formats list, which record-key is now present in.

if [".", ".."].contains(&s.as_str()) {
Err("Disallowed rkey")
} else if !RE_RKEY
.get_or_init(|| Regex::new(r"^[a-zA-Z0-9.\-_:~]{1,512}$").unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked that this matches the updated allowed character set, and that \- behaves correctly here.

@@ -556,7 +555,8 @@ fn string_type(string: &LexString) -> Result<(TokenStream, TokenStream)> {
Some(LexStringFormat::Handle) => quote!(crate::types::string::Handle),
Some(LexStringFormat::Nsid) => quote!(crate::types::string::Nsid),
Some(LexStringFormat::Language) => quote!(crate::types::string::Language),
// TODO: other formats
Copy link
Contributor

Choose a reason for hiding this comment

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

at-uri and uri are still not parsed here yet, so I think this TODO still applies (unless we specifically don't want to parse them here).

/// [Timestamp Identifier]: https://atproto.com/specs/record-key#record-key-type-tid
#[derive(Clone, Debug, PartialEq, Eq, Serialize)]
#[serde(transparent)]
pub struct Tid(String);
Copy link
Contributor

Choose a reason for hiding this comment

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

Document this as "Added" in the changelog.

/// A record key (`rkey`) used to name and reference an individual record within the same
/// collection of an atproto repository.
#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)]
pub struct RecordKey(String);
Copy link
Contributor

Choose a reason for hiding this comment

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

Document this as "Added" in the changelog.

/// A record key (`rkey`) used to name and reference an individual record within the same
/// collection of an atproto repository.
#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize)]
pub struct RecordKey(String);
Copy link
Contributor

Choose a reason for hiding this comment

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

Document this as "Removed" in the changelog, with a "(moved to string::RecordKey)" note.

@sugyan
Copy link
Owner Author

sugyan commented Apr 18, 2024

@str4d Thanks for reviewing! I updated comments and changelogs.

@sugyan sugyan merged commit 7a1a070 into main Apr 18, 2024
9 checks passed
@github-actions github-actions bot mentioned this pull request Apr 18, 2024
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.

Add new string formats
2 participants