Copy doc comments from Rust to JS #265
Conversation
|
@FreeMasen let me know when this is ready for review! |
|
@fitzgen This is ready for review. Let me know if you would like the additional 2 items I listed above |
There was a problem hiding this comment.
Thanks so much @FreeMasen! This is a super valuable feature.
In the future, if you're going to split up the PR into many commits, it would help me as a reviewer if the commits had descriptive messages and each commit logically did one thing. If it is too hard to split up logically like that, then it is easier for me if everything is one commit (although nice commit messages are still preferred!)
I have a couple nitpicks below, and once they're addressed, then we can merge this.
Thanks again!
crates/backend/src/ast.rs
Outdated
| None | ||
| } | ||
| }) | ||
| .fold(vec![], |mut acc, a| {acc.extend(a); acc}); |
There was a problem hiding this comment.
This should be a call to extract_doc_comments, right?
There was a problem hiding this comment.
yep, this has been updated
crates/backend/src/ast.rs
Outdated
| .iter() | ||
| .filter_map(|a| { | ||
| //We only care about outer comments for now | ||
| if a.style == AttrStyle::Outer { |
There was a problem hiding this comment.
Shouldn't this also check that the attributes are doc? It seems to me that this will collect non-doc attributes as well, which we don't want.
There was a problem hiding this comment.
Good catch, I am pretty new at using syn/proc-macro2, I updated this to check the path segments for a "doc" Ident instead of just capturing outer attributes. Let me know if you know of a better way to differentiate doc comment attributes.
crates/backend/src/ast.rs
Outdated
| @@ -1072,3 +1105,24 @@ fn replace_self(name: &Ident, item: &mut syn::ImplItem) { | |||
|
|
|||
| syn::visit_mut::VisitMut::visit_impl_item_mut(&mut Walk(name), item); | |||
| } | |||
| /// Extract the documentation comments from a Vec of attributes | |||
| fn extract_doc_comments(attrs: &Vec<syn::Attribute>) -> Vec<String> { | |||
There was a problem hiding this comment.
Is there a reason this needs to take &Vec<syn::Attribute> rather than &[syn::Attribute]? If not, then we should switch to the latter.
There was a problem hiding this comment.
There is not reason, just updated it
examples/comments/README.md
Outdated
| @@ -0,0 +1 @@ | |||
| # TODO | |||
There was a problem hiding this comment.
sorry, I think this was from an older commit it does have content.
tests/all/comments.rs
Outdated
| p.gen_bindings(&root, &target); | ||
| let js = p.read_js(); | ||
| let comments = extract_doc_comments(&js); | ||
| /// |
There was a problem hiding this comment.
Was there supposed to be something here?
There was a problem hiding this comment.
nope, not sure how this got there, it has been removed. I also updated the test to handle white-space a little better
|
I believe I have addressed all of you comments. Sorry about the commits, I will work on trying to break them apart a little better moving forward or just squash them all down. |
Addresses #57
Still to do:
optional additions
jsdoccomments@param {*} [name]@returns {*}