-
Notifications
You must be signed in to change notification settings - Fork 159
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
chore(*): Upgrade to hyper-0.14.x #459
Conversation
Thanks for contributing! Unfortunately, I'm here to tell you there were style issues with your Pull Request:
Guidelines are available at https://github.com/nickel-org/nickel.rs/blob/master/contributing.md This message was auto-generated by https://gitcop.com |
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 only trouble spots I see, from most meaningful to least, are:
- there's a lot of TODO for this refactor still present
- it's not clear that they're all addressed
- the loss of plugins is concerning
- I'm especially concerned about
multipart
- I don't see
cookies
/nickel-cookies
as a builtin either
- I'm especially concerned about
- there's some redundant interfaces that should be dropped (
body_parser
, for example) listener.socket()
appears gone, and this is a mildly interesting method to keeporigin
has becomepub
which is probably fine but will be conceptually hard to make private again later if need be- SSL support is moved into
hyper
or otherwise gone? - various startup/shutdown interface stuff
output_on_listen
tested and used in some places where it's not actually making anything cleareroutput_on_listen
not being used in some places where the listen socket is printedend
is now a no-op
Overall, it looks good to me, though this would benefit from another pass to remove dead code and improve idioms.
Finally, I'm glad to see the 'mw
lifetime go away but it's not obvious to me why <D: '_, '_>
is used. Is this inherited from hyper
changes?
Thanks for contributing! Unfortunately, I'm here to tell you there were style issues with your Pull Request:
Guidelines are available at https://github.com/nickel-org/nickel.rs/blob/master/contributing.md This message was auto-generated by https://gitcop.com |
Thank you so much for your continued support 🙏 |
You're welcome. A lot to be done before a 0.12 can release, though. |
At long last, we have an async version. This PR builds, and all but one example program works and have been tested (except the long broken https example). This is complete enough that the remaining work can be broken into more manageable pieces. There are some limitations:
cargo test
fails horribly. This PR is already large, and the integration tests require rewriting the test harness, so I plan on fixing the tests in the next PR. I could be persuaded otherwise though.The following are all for a future PR.
Middleware
implementation. This should be mostly a case of figuring out the correct type signature.middleware
androuter
macros do not support async. This is more complex to fix. We may want to combine this with a redesign of themiddleware
macro, which has confusing behavior. This is why themacro_example
program is disabled.These two items mean the only way to write an async middleware is to implement the
Middleware
trait. A number of the example programs demonstrate how to do this.plugins
are disabled due to the plugin crate not not being able to be restricted toSync + Send
. Theplugin
crate may be abandoned, so fixing this may be a headache.Response::on_send
is currently disabled. Feedback on its use cases is needed, in particular does it need to be async, which is more complex.