Skip to content

WIP: doc comments #2

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.

Sign up for GitHub

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

Open
polachok wants to merge 1 commit into
base: master
Choose a base branch
from
Open

WIP: doc comments #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions magnet_derive/src/codegen_enum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ pub fn impl_bson_schema_enum(attrs: Vec<Attribute>, ast: DataEnum) -> Result<Tok
Some(s) => Some(meta_value_as_str(&s)?.parse()?),
None => None,
};
let doc = doc_meta(&attrs).and_then(|doc| meta_value_as_str(&doc).ok());
let doc = quote! {
"description": #doc
};
let tagging = SerdeEnumTag::from_attrs(&attrs)?;

let variants: Vec<_> = ast.variants
Expand All @@ -25,6 +29,7 @@ pub fn impl_bson_schema_enum(attrs: Vec<Attribute>, ast: DataEnum) -> Result<Tok

let tokens = quote! {
doc! {
#doc,
"anyOf": [ #(#variants,)* ]
}
};
Expand Down
27 changes: 19 additions & 8 deletions magnet_derive/src/codegen_field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub fn impl_bson_schema_fields_extra(
impl_bson_schema_named_fields(attrs, fields.named, extra)
},
Fields::Unnamed(fields) => {
impl_bson_schema_indexed_fields(fields.unnamed, extra)
impl_bson_schema_indexed_fields(attrs, fields.unnamed, extra)
},
Fields::Unit => {
assert!(extra.is_none(), "internally-tagged unit should've been handled");
Expand All @@ -51,10 +51,17 @@ fn impl_bson_schema_named_fields(
) -> 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());
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

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! {}
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Author

@polachok polachok Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

oh, that is quite right. I didn't notice that. Nevermind, then. :)

Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

};
let tokens = if let Some(TagExtra { tag, variant }) = extra {
quote! {
doc! {
"type": "object",
#doc
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

(I mean, add a comma here…)

"additionalProperties": false,
"required": [ #tag, #(#properties,)* ],
"properties": {
Expand All @@ -67,6 +74,7 @@ fn impl_bson_schema_named_fields(
quote! {
doc! {
"type": "object",
#doc
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

(…and here.)

"additionalProperties": false,
"required": [ #(#properties,)* ],
"properties": {
Expand All @@ -90,15 +98,17 @@ fn field_def(field: &Field) -> Result<TokenStream> {
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);
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

Please propagate the error here as well, instead of using .ok().

Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, .unwrap_or_else(String::new) can be simplified to .unwrap_or_default().


Ok(quote! {
::magnet_schema::support::extend_schema_with_bounds(
<#ty as ::magnet_schema::BsonSchema>::bson_schema(),
::magnet_schema::support::Bounds {
lower: #lower,
upper: #upper,
},
)
::magnet_schema::support::extend_schema_with_doc(
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

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.

::magnet_schema::support::extend_schema_with_bounds(
<#ty as ::magnet_schema::BsonSchema>::bson_schema(),
::magnet_schema::support::Bounds {
lower: #lower,
upper: #upper,
},
), #doc)
})
}

Expand Down Expand Up @@ -163,6 +173,7 @@ fn field_names(attrs: &[Attribute], fields: &Punctuated<Field, Comma>) -> Result
/// Implements `BsonSchema` for a tuple `struct` or variant,
/// with unnamed (numbered/indexed) fields.
fn impl_bson_schema_indexed_fields(
attrs: &[Attribute],
mut fields: Punctuated<Field, Comma>,
extra: Option<TagExtra>,
) -> Result<TokenStream> {
Expand Down
15 changes: 15 additions & 0 deletions magnet_derive/src/meta.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,21 @@ use std::f64;
use syn::{ Attribute, Meta, NestedMeta, MetaNameValue, Lit };
use error::{ Error, Result };

pub fn doc_meta(attrs: &[Attribute]) -> Option<MetaNameValue> {
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

Please add doc comments to all items (especially public ones). Running cargo +nightly clippy can show you wherever they are missing.

let mut value = attrs.iter().filter_map(|attr| {
if let Some(meta) = attr.interpret_meta() {
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

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,
})

match meta {
Meta::NameValue(ref val) if val.ident == "doc" => {
return Some(val.clone())
}
_ => {},
};
}
None
});
value.next()
}

/// Returns the inner, `...` part of the first `#[name(...)]` attribute
/// with the specified name (like `#[magnet(key ( = "value")?)]`).
/// TODO(H2CO3): check for duplicate arguments and bail out with an error
Expand Down
8 changes: 8 additions & 0 deletions magnet_schema/src/support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ pub fn extend_schema_with_bounds(mut schema: Document, bounds: Bounds) -> Docume
schema
}

#[doc(hidden)]
Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

(Therefore, this function will not be necessary, its body will be incorporated into extend_schema().)

pub fn extend_schema_with_doc(mut schema: Document, doc: &str) -> Document {
if !doc.is_empty() {
schema.insert("description", doc.trim_left());
}
schema
}

Copy link
Owner

@H2CO3 H2CO3 Jul 25, 2018

Choose a reason for hiding this comment

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

I think here emptiness should be tested after trimming the doc-string.

/// This function should not be used directly; calls to it are only generated by
/// `magnet_derive` when emitting code for internally-tagged newtype variants.
///
Expand Down