-
Notifications
You must be signed in to change notification settings - Fork 9
add an impl_typed_uuid_kinds macro with x-rust-type support #71
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 an impl_typed_uuid_kinds macro with x-rust-type support #71
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Got this working end to end in Omicron with this patch: https://gist.github.com/sunshowers/1484f8cfa249c44c1426728194a258b6 |
Created using spr 1.3.6-beta.1
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.
This is really beautiful.
/// | ||
/// By default, for a kind `Foo`, this macro generates: | ||
/// | ||
/// * A `FooKind` type that implements `TypedUuidKind`: `pub type FooKind {}`. |
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.
/// * A `FooKind` type that implements `TypedUuidKind`: `pub type FooKind {}`. | |
/// * A `FooKind` type that implements `TypedUuidKind`: `pub enum FooKind {}`. |
?
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.
Thanks, fixed.
/// | ||
/// // This generates empty UserKind and OrganizationKind enums implementing | ||
/// // TypedUuidKind, with the tags "user" and "organization" respectively. | ||
/// // Tags are snake_case versions of type names. |
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.
You might consider an example that demonstrates the snakiness of the casing e.g. BusinessUnit rather than Organization ("business_unit")
Is that PascalCasiness a constraint on the inputs or only a suggestion?
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.
/// * The `Kind` types are all empty (uninhabited) enums, which means that | ||
/// values for these types cannot be created. (Using empty enums is the | ||
/// recommended approach for `newtype-uuid`). |
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 haven't heard "uninhabited", but I have used "marker" I think in case that's a useful shibboleth?
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.
"Uninhabited" is a pretty Haskell-like term I guess. I'll change this to "also known as marker or uninhabited enums".
/// attrs = [#[cfg(feature = "schemars")]], | ||
/// rust_type = { | ||
/// crate = "my-crate", | ||
/// version = "*", |
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.
may I request that you use an explicit version here? I worry about copy-pasta proliferation that "works" but might be the wrong thing.
/// version = "*", | |
/// version = "0.1.0", |
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.
Reasonable, though I'm wondering if we should document something like "if your list of kinds is append-only, consider using "*"
". Probably too much detail for this documentation, though.
use ::#newtype_uuid_crate::macro_support::schemars08::schema::*; | ||
|
||
let mut schema = SchemaObject { | ||
instance_type: ::std::option::Option::None, |
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.
do you need this? or just here to be explicit?
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.
Ah yeah I don't think this is necessary, removed.
#[cfg(feature = "alloc")] | ||
extern crate alloc; | ||
|
||
/// Macro support for [`newtype-uuid-macros`]. |
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.
do you want to make it clear that this is unstable? I might have considered making it doc = hidden
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.
Yeah definitely should be hidden in docs.
crates/newtype-uuid/src/lib.rs
Outdated
/// Returns an alias for `TypedUuid<Self>`, if one is defined. | ||
/// | ||
/// The alias must be defined in the same module as `Self`. This alias is | ||
/// used by schemars integration to refer to `TypedUuid<Self>` if available. |
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 needed to reread this a couple of times to understand it. In particular this means a type alias e.g. pub type = TypedUuid
; perhaps this would have been clearer to me:
/// Returns an alias for `TypedUuid<Self>`, if one is defined. | |
/// | |
/// The alias must be defined in the same module as `Self`. This alias is | |
/// used by schemars integration to refer to `TypedUuid<Self>` if available. | |
/// Returns a string that corresponds to a type alias for `TypedUuid<Self>`, if one is defined. | |
/// | |
/// The type alias must be defined in the same module as `Self`. This function is | |
/// used by the schemars integration to embed a reference to that alias in the schema. |
Might it make sense to mention the macro here as well? While folks can certainly use this independent of the macro, it might be worth guiding them toward the easy / intended path.
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.
Yeah, makes sense.
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
This PR adds a new function-style proc macro called
impl_typed_uuid_kinds
, with support for automatic replacement of UUID kinds in typify (and therefore progenitor).A typical invocation is:
TODO:
(Updates to Progenitor are not necessary since the fixes are all in typify.)
Update Crucible: update progenitor to 0.11.0 crucible#1762Update Propolis: update progenitor to 0.11.0 propolis#931Update Progenitor in Omicron (TODO)