New schema; no codegen yet #2495
Conversation
aumetra
force-pushed
the
aw/only-schema
branch
2 times, most recently
from
2e2323f to
d039224
Compare
| where | ||
| T: ?Sized, | ||
| { | ||
| // Taken from <sagebind/castaway>: https://github.com/sagebind/castaway/blob/a7baeab32d75d0f105d1415210a2867d213f8818/src/utils.rs#L36 |
There was a problem hiding this comment.
Wild stuff. At first, I wasn't quite sure that the transmute is fine, but should be ok given that it's transmuting a PhantomData that is never used.
There was a problem hiding this comment.
yeah, its basically the implementation that's currently in castaway. checked through it and it seems to be fine. just a cast to 'static so the lifetimes can be ignored.
There was a problem hiding this comment.
and yep, due to the hacky nature the id is intentionally completely opaque. meaning if there's a better/less hacky way possible, it can be swapped out in future releases without any semver breakage
There was a problem hiding this comment.
(i mean for peace of mind we can also run the tests for cw-schema with miri. that way we can be sure it doesn't produce any UB)
There was a problem hiding this comment.
Just today, I found this crate. Maybe it's better to use that? The underlying implementation seems to be the same, but I would assume dtolnay would notice earlier than us if a problem with that implementation was found.
|
|
||
| const DISABLE_WARNINGS_VAR: &str = "SHUT_UP_CW_SCHEMA_DERIVE"; | ||
|
|
||
| fn print_warning(title: impl Display, content: impl Display) -> io::Result<()> { |
There was a problem hiding this comment.
Is it possible to add a code location to the warning?
There was a problem hiding this comment.
yeah. probably from the span. gonna look into that
| #[doc(hidden)] | ||
| pub use self::idl::JsonCwApi; |
There was a problem hiding this comment.
Is the reexport needed? It doesn't seem to be used anywhere.
| @@ -70,13 +72,21 @@ pub trait QueryResponses: JsonSchema { | |||
| } | |||
|
|
|||
| fn response_schemas_impl() -> BTreeMap<String, RootSchema>; | |||
|
|
|||
| fn response_schemas_cw() -> Result<BTreeMap<String, cw_schema::Schema>, IntegrityError> { | |||
There was a problem hiding this comment.
Why is there even a Result here if it never fails?
There was a problem hiding this comment.
just copied the above definitions to duplicate and tweak it as a quick hack to get the automatic codegen working. but agreed, the errors can be omitted (on both schemas actually)
| /*print_warning( | ||
| "unknown serde attribute", | ||
| format!( | ||
| "unknown attribute \"{}\"", | ||
| meta.path | ||
| .get_ident() | ||
| .map(|ident| ident.to_string()) | ||
| .unwrap_or_else(|| "[error]".into()) | ||
| ), | ||
| ) | ||
| .unwrap();*/ |
There was a problem hiding this comment.
With this commented out, it would be easy to miss incorrect schema being generated, but it's also quite annoying to get lots of warnings.
Can we add some attribute to suppress the warnings locally (on the field or on the container level?). Like #[allow(unknown_serde_attr)]? Only having the env var seems a bit coarse.
There was a problem hiding this comment.
yeah, an attribute would be possible. didn't think about that tbh, gonna implement that
| ) | ||
| .unwrap();*/ | ||
|
|
||
| // TODO: support other serde attributes |
There was a problem hiding this comment.
Can we create an issue to track this (and the other TODO for SerdeFieldOptions)?
I dunno. I mean the PR is huge lines wise, yeah, but most of that is lockfiles being regenerated and schema files that have to be generated. Kinda necessary.
The codegen will be PR'd separately in an attempt to make it kinda smaller focus-wise?