Issue #3 and #4: Add field documentation and annotation #6
Conversation
|
I tried to move the While it seems possible to go down that route, it might not be as useful as I though. For example rustdoc will not build doc for tuple struct fields (see rust-lang/rust#42615). |
nbigaouette
force-pushed
the
issue-3-and-4-field-documentation-annotation
branch
from
de0d171 to
3e44fb6
Compare
src/lib.rs
Outdated
| } | ||
|
|
||
| /// Set field's documentation. | ||
| pub fn with_documentation(&mut self, documentation: &str) -> &mut Self { |
There was a problem hiding this comment.
Would you mind naming this doc to match the other functions? If you want, we can bikeshed naming patterns in a separate issue.
src/lib.rs
Outdated
| } | ||
|
|
||
| /// Set field's annotation. | ||
| pub fn with_annotation(&mut self, annotation: &str) -> &mut Self { |
There was a problem hiding this comment.
Could you name this annotation? I'm avoiding with_ prefixes.
src/lib.rs
Outdated
| } | ||
| _ => panic!("field list is named"), | ||
| } | ||
| }; |
There was a problem hiding this comment.
I don't think that this semicolon is needed.
There was a problem hiding this comment.
Hum! interesting. I though the match would return... Fixed in 95b6673.
src/lib.rs
Outdated
| documentation: String, | ||
|
|
||
| /// Field annotation | ||
| annotation: String, |
There was a problem hiding this comment.
I think this probably should be a Vec<String> to support adding multiple annotations. In this case, the annotation builder function would push a new string.
src/lib.rs
Outdated
| /// Field name | ||
| name: String, | ||
|
|
||
| /// Field type | ||
| ty: Type, | ||
|
|
||
| /// Field documentation | ||
| documentation: String, |
There was a problem hiding this comment.
Documentation strings are tricky. Reading the diff, it looks like only a single doc string line is supported. I'm not sure what the best option would be to support multi lines. Either "\n" could be detected in the documentation string and then it all gets formatted, or documentation could be a Vec<String>.
Thoughts"
There was a problem hiding this comment.
How about different functions? Have doc() take a Vec<String> to stay the same as annotation() and add something like doc_with_newlines() which takes a String.
|
Yes, using a |
|
@nbigaouette ping. Any chance we can get this landed? |
…ti-line annotation
|
Wow, that slipped my mind, sorry! :D I've replaced the I guess something similar could be done for the |
|
Poke again :-D |
…multi-line documentation
|
Any news? |
|
Poke? Would be really great if this could get merged |
| } | ||
|
|
||
| /// Set field's annotation. | ||
| pub fn annotation(&mut self, annotation: Vec<&str>) -> &mut Self { |
There was a problem hiding this comment.
Maybe annotation(&mut self, annotation: &str) like import(&mut self, path: &str, ty: &str)?
There was a problem hiding this comment.
I'm not sure I follow. Do you mean using a &str instead of a Vec<&str> for the annotation() method?
It was changed to use a Vec in 6e17dc2 to support multiple lines.
There was a problem hiding this comment.
I meant a method to add one annotation line per call, like you do for import(...) - each call results in a new import - instead of passing the whole Vec with all lines :)
| } | ||
|
|
||
| /// Set field's documentation. | ||
| pub fn doc(&mut self, documentation: Vec<&str>) -> &mut Self { |
There was a problem hiding this comment.
Maybe doc(&mut self, documentation: &str) like import(&mut self, path: &str, ty: &str)?
|
@carllerche Would have been awesome to have this in, as I was going to create an issue. I really like this lib, but if it's abandoned, a short note on the README would be handy. |
|
Another ping here, can we get this merged? |
andrewleverette
dismissed
carllerche’s stale review
Requested change has been made
This PR adds two fields to the
Fieldstruct to set a struct field's documentation and annotation.The
Fieldtype must bepubto be created outside of the crate.A
Struct::push_field()function is added to add the manually created field to the struct.I also split
Fields::named()intonamed()andpush_named()(see 6093af4) to be able to reuse the latter.This PR is a work in progress. I haven't added the modification to tuple fields since their content is a different type (
Typeinstead ofField). I'd like to make sure this is a proper path before continuing. Maybe the doc and annotation could go inside theTypeinstead?Should close #3 and #4.