Response references, IntoResponseComponent and IntoResponses #215
Conversation
src/lib.rs
Outdated
| /// } | ||
| /// } | ||
| /// ``` | ||
| pub trait ResponseComponent { |
There was a problem hiding this comment.
Just wondering should this actually just be Response instead of a ResponseComponent? Component response seems quite long name feels like little bit stuttering? Although if the name is just a Response it easily will conflict with other crates which offer types with same name.
Also wondering should the Component derive trait be renamed to Schema but it might be too big breaking change for people already using the library. Basically they would have to change it to every type they previously had Component
There was a problem hiding this comment.
Also wondering should the Component derive trait be renamed to Schema but it might be too big breaking change for people already using the library. Basically they would have to change it to every type they previously had Component
I was also tempted by this change, it would be good to make for the sake of clarity and consistency, but perhaps too big of a break? At least with renames it's usually not too difficult to fix with a grep + sed style find replace.
There was a problem hiding this comment.
Just wondering should this actually just be Response instead of a ResponseComponent? Component response seems quite long name feels like little bit stuttering? Although if the name is just a Response it easily will conflict with other crates which offer types with same name.
Sure I would be happy to rename it. If it's a conflict people can import it with a rename or with a module scope 🙂
There was a problem hiding this comment.
I was also tempted by this change, it would be good to make for the sake of clarity and consistency, but perhaps too big of a break? At least with renames it's usually not too difficult to fix with a grep + sed style find replace.
Yeah changing it within utoipa should be quite painless change except those 50 + breaking tests. 😄 Since it is also a breaking change and if wanted to make it is in fact good to do now, because there are other breaking changes pending in this next release. Then it just need to be documented to the #181 discussion. I guess we could just do it too? Should it be another PR though?
Sure I would be happy to rename it. If it's a conflict people can import it with a rename or with a module scope
Lest rename it 🙂
There was a problem hiding this comment.
Let's do it in a subsequent PR, seeing as it it is a rather large change!
There was a problem hiding this comment.
That looks good, do you think it might be good to represent this structure in the underlying data types using something like a ComponentsBuilder?
If something then the Comonent references should be probably renamed to Schemas. And similarly the function components_from_iter and component should be renamed to schemas_from_iter and to schema. Then it would follow the spec more closely.
There was a problem hiding this comment.
Perhaps
SchemaGeneratoror something with a similar meaning
Yeah the SchemaGenerator indicate that it creates a generator object. Perhaps GenerateSchema but again it would be unnecessary long or GenSchema? Like serde's Serialize or Deserialize what would be if possible one word or as short as possible descriptive action that it would perform. Rust prefers naming traits as one verb but also or and er endings are somewhat common too. I guess ToSchema and ToResponse would be not too far fetched and sound quite reasonable?
There was a problem hiding this comment.
components_from_iter and component should be renamed to schemas_from_iter and to schema. Then it would follow the spec more closely.
I've made that change in the PR for ComponentsBuilder, because it stores the components in a variable called schemas already, so I made it match.
There was a problem hiding this comment.
Yeah the
SchemaGeneratorindicate that it creates a generator object. PerhapsGenerateSchemabut again it would be unnecessary long orGenSchema? Like serde'sSerializeorDeserializewhat would be if possible one word or as short as possible descriptive action that it would perform. Rust prefers naming traits as one verb but alsooranderendings are somewhat common too. I guessToSchemaandToResponsewould be not too far fetched and sound quite reasonable?
I think using the full word is usually better than shortcuts because it makes the code more accessible to those who don't speak English natively (and are using translation to understand), or those who are using screen readers or dictation to work with the code. We should optimize for reading. But ToSchema and ToResponse seem like a reasonable option to me, let's go with that. I've renamed the ResponseComponent to ToResponse. I can do the Component to ToSchema rename in the next PR.
Is this PR ready now do you think?
There was a problem hiding this comment.
Yeah you are right.. Let's get the ball rolling and get this thing to the upstream. Thanks for your work ❤️
Close #201
TODO:
Implement thethis can be done in a separate PRresponses( (code = "200", response = MyResponse) )as per IntoResponses trait #201 (comment) if we decide we want it.This work was sponsored by Arctoris.