-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
@str4d could you review this? |
There was a problem hiding this 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()) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
@str4d Thanks for reviewing! I updated comments and changelogs. |
Resolve #154.
ref: https://atproto.com/specs/record-key
The
RecordKey
was originally defined by @str4d intypes.rs
, but has been moved tostring.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.