WIP: doc comments #2
Conversation
| } | ||
| schema | ||
| } | ||
|
|
There was a problem hiding this comment.
I think here emptiness should be tested after trimming the doc-string.
| ) -> Result<TokenStream> { | ||
| let properties = &field_names(attrs, &fields)?; | ||
| let defs: Vec<_> = fields.iter().map(field_def).collect::<Result<_>>()?; | ||
| let doc = doc_meta(&attrs).and_then(|doc| meta_value_as_str(&doc).ok()); |
There was a problem hiding this comment.
Please do not use Result::ok() here – attribute parsing errors are meant to be propagated upward.
| let doc = if doc.is_some() { | ||
| quote! { "description": #doc.trim_left(), } | ||
| } else { | ||
| quote! {} |
There was a problem hiding this comment.
I think this would be better served by
doc.map(|doc| quote! { "description": #doc.trim_left(), }).unwrap_or_default()
Also, the trailing comma should be at the use site for better readability.
There was a problem hiding this comment.
hm, but if #doc is empty it would error out for extra comma.
i guess i can do "description": #doc, so description would be there just empty
There was a problem hiding this comment.
oh, that is quite right. I didn't notice that. Nevermind, then. :)
There was a problem hiding this comment.
However, that should also be true for the other similar places, then. For example, when doc == None, then wouldn't this also be a syntax error: codegen_enum.rs:19-22? I think that case might also be better written as mapping the whole quoted token stream over the optional doc comment, then unwrap_or_default()ing it at the end for an empty token stream id the comment is missing.
| quote! { | ||
| doc! { | ||
| "type": "object", | ||
| #doc |
| quote! { | ||
| doc! { | ||
| "type": "object", | ||
| #doc |
| upper: #upper, | ||
| }, | ||
| ) | ||
| ::magnet_schema::support::extend_schema_with_doc( |
There was a problem hiding this comment.
Instead of wrapping each kind of extension into its own function, and increasing nesting, let's create a single extension context object and an extend_schema() function. The extension context object could contain all the potential extensions as optionals, and extend_schema() could apply them in one go. This sounds to me like a more principled and easier-to-understand approach.
| schema | ||
| } | ||
|
|
||
| #[doc(hidden)] |
There was a problem hiding this comment.
(Therefore, this function will not be necessary, its body will be incorporated into extend_schema().)
| let max_excl = magnet_meta_name_value(&field.attrs, "max_excl")?; | ||
| let lower = bounds_from_meta(min_incl, min_excl)?; | ||
| let upper = bounds_from_meta(max_incl, max_excl)?; | ||
| let doc = doc_meta(&field.attrs).and_then(|doc| meta_value_as_str(&doc).ok()).unwrap_or_else(String::new); |
There was a problem hiding this comment.
Please propagate the error here as well, instead of using .ok().
There was a problem hiding this comment.
Also, .unwrap_or_else(String::new) can be simplified to .unwrap_or_default().
| use syn::{ Attribute, Meta, NestedMeta, MetaNameValue, Lit }; | ||
| use error::{ Error, Result }; | ||
|
|
||
| pub fn doc_meta(attrs: &[Attribute]) -> Option<MetaNameValue> { |
There was a problem hiding this comment.
Please add doc comments to all items (especially public ones). Running cargo +nightly clippy can show you wherever they are missing.
|
|
||
| pub fn doc_meta(attrs: &[Attribute]) -> Option<MetaNameValue> { | ||
| let mut value = attrs.iter().filter_map(|attr| { | ||
| if let Some(meta) = attr.interpret_meta() { |
There was a problem hiding this comment.
This can be simplified to:
attr.interpret_meta().and_then(|meta| match meta {
Meta::NameValue(ref val) if val.ident == "doc" => Some(val.clone()),
_ => None,
})
|
Thanks for taking the time to work on this! Awesome! 👍 |
2 participants
Started working on #1