-
-
Notifications
You must be signed in to change notification settings - Fork 36
refactor!: extract framework core to cot-core #444
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
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
a799923 to
c1c890a
Compare
|
The nightly error should be fixed by rust-lang/rust#150939 |
0033822 to
ead28ff
Compare
m4tx
left a 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.
Looks nice, and seems like it lays a solid foundation for our eventual extraction of the ORM to a separate crate!
I've tried to generate the docs with just docs and it seems like there's a ton of warnings coming from cot-core (outside of missing examples, which are expected) - please make sure these are fixed before we consider merging this.
e364514 to
6a3a723
Compare
6a3a723 to
9b372af
Compare
ced97e4 to
0536013
Compare
b7b3ec3 to
9fd687f
Compare
9fd687f to
b3e3be5
Compare
# Conflicts: # Cargo.toml
b3e3be5 to
cb940a3
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.
Pull request overview
Copilot reviewed 41 out of 47 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
m4tx
left a 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.
Looks amazing - in the thorough review, I found a few documentation issues; when you fix them, please feel free to merge this!
| /// Extractors implementing this trait are used in route handlers that consume | ||
| /// the request body and therefore can only be used once per request. | ||
| /// | ||
| /// See [`crate::request::extractors`] documentation for more information about |
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 redirects to cot_core docs instead of cot.
| impl_into_cot_error!(JsonDeserializeError, BAD_REQUEST); | ||
|
|
||
| /// An extractor that gets the request body as form data and deserializes it | ||
| /// into a type `F` implementing `cot::form::Form`. |
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 hasn't changed in your change, but while we're at it, please make cot::form::Form an actual link.
| /// | ||
| /// If you need to consume the body of the request, use [`FromRequest`] instead. | ||
| /// | ||
| /// See [`crate::request::extractors`] documentation for more information about |
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 redirects to cot_core docs instead of cot.
| if let Some(not_found) = error.inner().downcast_ref::<NotFound>() { | ||
| use crate::error::NotFoundKind as Kind; | ||
| match ¬_found.kind { | ||
| Kind::FromRouter => {} |
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.
Please add this match variant back for clarity, or at least explicitly document that we don't do anything in case we get FromRouter here.
| #[doc(inline)] | ||
| pub use cot_core::error::{MethodNotAllowed, UncaughtPanic}; | ||
| #[doc(inline)] | ||
| pub(crate) use cot_core::error::{backtrace, impl_into_cot_error}; |
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.
If it's pub(crate) anyway, what's the point in re-exporting this? Can't we use cot_core directly instead?
| //! | ||
| //! Cot uses the [`Request`](http::Request) type from the [`http`] crate | ||
| //! to represent incoming HTTP requests. However, it also provides a | ||
| //! [`RequestExt`](../../cot/request/trait.RequestExt.html) trait that contain | ||
| //! various helper methods for working with HTTP requests. These methods are | ||
| //! used to access the application context, project configuration, path | ||
| //! parameters, and more. You probably want to have a `use` statement for | ||
| //! [`RequestExt`](../../cot/request/trait.RequestExt.html) in your code most of | ||
| //! the time to be able to use these functions: | ||
| //! | ||
| //! ``` | ||
| //! use cot::request::RequestExt; | ||
| //! ``` |
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 think we can now remove this, as there's a copy of this in the main crate (and if we don't remove it here, it will inevitably go out of sync eventually).
| //! | ||
| //! Cot uses the [`Response`](http::Response) type from the [`http`] crate | ||
| //! to represent outgoing HTTP responses. However, it also provides a | ||
| //! [`ResponseExt`] trait that contain various helper methods for working with | ||
| //! HTTP responses. These methods are used to create new responses with HTML | ||
| //! content types, redirects, and more. You probably want to have a `use` | ||
| //! statement for [`ResponseExt`] in your code most of the time to be able to | ||
| //! use these functions: | ||
| //! | ||
| //! ``` | ||
| //! use cot::response::ResponseExt; | ||
| //! ``` |
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.
Same here - we can remove this.
| /// An extractor that extracts data from the URL params. | ||
| /// | ||
| /// The extractor is generic over a type that implements | ||
| /// `serde::de::DeserializeOwned`. |
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.
| /// `serde::de::DeserializeOwned`. | |
| /// [`serde::de::DeserializeOwned`]. |
I know you didn't change this, but it's a good opportunity to make this better.
| /// An extractor that extracts data from the URL query parameters. | ||
| /// | ||
| /// The extractor is generic over a type that implements | ||
| /// `serde::de::DeserializeOwned`. |
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.
| /// `serde::de::DeserializeOwned`. | |
| /// [`serde::de::DeserializeOwned`]. |
| impl_into_cot_error!(QueryParametersParseError, BAD_REQUEST); | ||
|
|
||
| /// Extractor that gets the request body as JSON and deserializes it into a type | ||
| /// `T` implementing `serde::de::DeserializeOwned`. |
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.
| /// `T` implementing `serde::de::DeserializeOwned`. | |
| /// `T` implementing [`serde::de::DeserializeOwned`]. |
|
By the way, asking out of curiosity - did you try to find out why the semver-checks is complaining? Seems like it produces a lot of false positives about modules/structures not accessible using old paths, which isn't true. I'm wondering if anyone has reported that yet (and if not, it's definitely worth reporting!) EDIT: ah, I guess this is obi1kenobi/cargo-semver-checks#638 |
This PR attempts to extract the core functionality of the framework to separate crate to speed up compilation among other reasons.
The modules fully/partially moved:
bodyerrorheadershtmljsonresponserequestmiddlewarehandlerI think moving
routerwould be valid move as well, but in the current format it's too intertwined with OpenAPI functionality (that we don't want to pull into core) that it can be done at a later time.With
cot-corebeing a separate crate, the following items had to become public:cot_core::error::backtrace- needed forErrorPagecot_core::error::error_impl::*- to allow implementing of errors incotError.backtrace()- needed for supplying error backtrace inErrorPageTemplateBuilderBacktrace.frames()- needed for theTemplatederive forErrorPageTemplate(used inerror.htmltemplate)StackFrame.symbol_name()- as aboveStackFrame.location()- as aboveStackFrame- as aboveBody.axum()- needed forrequest_axum_to_cotinproject.rsInvalidContentTypeAppNameRouteNameBoxRequestHandler- used inopenapi.rsandrouter.rsinto_box_request_handler- used inmethod.rsandrouter.rsStayed private with following gotchas:
NotFound.router()- removed deprecatedNotFoundconstructor methods fromError