-
-
Notifications
You must be signed in to change notification settings - Fork 347
feat(uri): make Authority/PathAndQuery::from_static const
#786
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?
feat(uri): make Authority/PathAndQuery::from_static const
#786
Conversation
2f36d19 to
a2f1fb4
Compare
Authority::from_static and PathAndQuery::from_static constAuthority/PathAndQuery::from_static const
|
Hello @seanmonstar This PR has been open for some time, and I'd like to politely request a review (mainly marking |
|
Thanks for the PR! I think the objective is worthy, for sure! I do worry about having parsing be duplicated in two different forms, it's possible for one to change without the other. Is it possible to make this change while keeping only one version of the parsing code? |
@seanmonstar Thanks for the feedback! IMO the clean solution is to extract the validation into a shared const helper function that both The refactor would look like:
I can implement this if it sounds good to you. Or do you have any good ideas? |
|
I think that makes sense, can the |
a2f1fb4 to
125440a
Compare
|
(I can address the MSRV issues in a separate PR.) |
|
I've fixed the MSRV, raising it to 1.57. If you want to change from using the weird panic syntax to just a plain string, we can merge this in for the next release :) |
Thanks! |
125440a to
9e11945
Compare
Changes
Authority::from_staticandPathAndQuery::from_staticare now const.Explanation
Uri::from_staticremains non-const. Two blockers under MSRV 1.49:&'static stris unavailable, but Uri parsing requires slicing the input into scheme/authority/path+query.Tests
Related issue
Partially fixes #750