-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
WIP: Implement auto-documenting routes #1624
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super excited for this. Some simplifications and forward-looking statements.
Thanks for submitting this early!
I think we have Rocket act as a data provider wherein Rocket allows any consumer to access all of the available documentation information. The information should be rich enough (richer, ideally!) to allow shipping standard doc consumers in Rocket directly, such as those for Effectively, Rocket's core should be unaware of any standard like
As above, Rocket itself should be agnostic to doc frontends as it should be for doc standards. We might consider shipping a fairing or custom handler or something of that sort of a few front-ends, whoever. In general, I envision the following pipeline:
Where Rocket's core participates only in the first stage and everything else can be implemented externally. Standards like
If global, auth will be a fairing, which implies fairings should be able to add docs. If local, auth will be a guard, which we already know should be able to add docs. It would be nice to be able to read the crate docstring, but that's not accessible easily. #[rocket(doc)]
/// Some docs to make global, or something. This doesn't work because one binary might have more than one Rocket server, and the docs need to be per-server. Oh! We could take the doc-string from the /// This is my application. Here are some nice docs.
///
/// These will show up somewhere.
#[launch]
fn rocket() -> _ { ... } |
6388abd
to
29a4175
Compare
I have created some types that can do some of the lifting when documenting a type (I'm 100% sure that they are not sufficient even for just OpenAPI, but lets start small). I think the next steps should be to modify Rocket to collect and aggregate the documentation that we have now gathered for each route and to create a macro that can derive the |
29a4175
to
32eff7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The different files for the different impls are unnecessary; they can and should all be in one file. Eventually, we want to dedup these by using macros, too.
In terms of Schema
, we should start with structures that are able to describe the entirety of the serde data model, though several of these can be coalesced when reified. I wouldn't be surprised if structures that do so already exist somewhere. There's Content
, which is a serialized version of this, and Figment's Value and this Value
, which is similar but excludes Option
, which we shouldn't. So, effectively something like these but without the actual values.
I don't think Schema
here makes too much sense. min
/max
don't make sense for many types, for example. I think any sort of value spec should be per-value.
In any case, the next step is to actually grab the schema for the route types much like is done now for sentinels minus the part about embeddings.
@@ -209,9 +211,11 @@ impl Route { | |||
}) | |||
.collect(); | |||
|
|||
let docstring = String::from_attrs("doc", &handler.attrs)?.join("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you determine if we need the \n
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we need it, because Rust comments are markdown, which is also supported for OpenApi and RAML. Markdown is a newline sensitive format: # title \n remainder\n
is not the same as # title remainder
, so removing the newline would alter the structure of the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is really whether rustdoc already inserts a new line or whether it infers one based on having multiple attributes. You should be able to easily check. We only need the new line here if it is inferred.
Gone
I have implemented HasSchema for every type in the serde data model, except for
I agree that some of the spec must come from the value, but some of it can also come from the type. Many auto documenters provide defaults like
So would this mean adding a field to StaticInfo with the ingoing types and the outgoing types? |
32eff7b
to
521216c
Compare
What I'm suggesting is that we have a struct/enum that we can use to dynamically determine the serde data type of a given Rust type. I don't see where we would need generics for this.
What I mean is that different data types have different "specs", so we shouldn't have one "spec" kind that applies to every data type.
Not quite. See how this is done with |
521216c
to
1f2729d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, progress here seems a bit without aim. Instead of working forward in an ambiguous direction, I'd like for us to set an ideal target. What would be the gold standard here? The answer should include:
- What the user has to do to render docs.
- What the API will look like to add a doc renderer.
- How the user can progressively document their app.
It would be ideal to work with a non-trivial example. Perhaps choose a real API, like the Bitwarden API or perhaps the Stripe API, imagine how endpoints might be written in Rocket, and then design a doc system, without concern for feasibility, that would allow rendering those kinds of docs as streamlined as possible.
@@ -319,6 +352,9 @@ fn codegen_route(route: Route) -> Result<TokenStream> { | |||
let rank = Optional(route.attr.rank); | |||
let format = Optional(route.attr.format.as_ref()); | |||
|
|||
// Get the doc comment | |||
let docstring = &route.docstring; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to separate this. Group it with the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean add it to the StaticInfo::schemas
field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I just need style wise. Basically to just remove the whitespace and comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right okay
@@ -209,9 +211,11 @@ impl Route { | |||
}) | |||
.collect(); | |||
|
|||
let docstring = String::from_attrs("doc", &handler.attrs)?.join("\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question is really whether rustdoc already inserts a new line or whether it infers one based on having multiple attributes. You should be able to easily check. We only need the new line here if it is inferred.
Set, | ||
} | ||
|
||
pub struct TypeSchema<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make much sense to me. name
seems to be a string serialization of the kind enum, so why have it at all? Or alternatively, why not have the trait simply have a const string?
I think we need to start thinking more seriously about how schema is going to be represented. Otherwise, we will be bouncing back-and-forth between ideas without justification. A good approach here is the take a reasonably sized application and imagine the exact documentation we would like to render and work backwards from there. We also have the benefit of existing schema like JSON schema and so on. 
1f2729d
to
1e7bcea
Compare
Very large WIP, does nothing for now.
This PR aims at eventually closing #297, or at least providing some of the requested functionality. For now I have created the traits
Documented
andUndocumented
, and I have updated the derive macro to pull in the doc comments of the route handler function. I am opening an early PR to get some feedback.Unresolved questions (non-exhaustive :)):
Route
.