Skip to content
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

upgrades rocket to 0.5 #132

Closed
wants to merge 1 commit into from
Closed

upgrades rocket to 0.5 #132

wants to merge 1 commit into from

Conversation

Maweypeyyu
Copy link

This changes/fixes/improves …

I license past and future contributions under the dual MIT/Apache-2.0 license, allowing licensees to chose either at their option.

Let me know if I can improve anything.

@Maweypeyyu Maweypeyyu mentioned this pull request Dec 30, 2021
1 task
Copy link
Owner

@HeroicKatora HeroicKatora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, looks like the next rocket version, when it is finally released…, will provide some interesting asynchronous wins. I wonder if it could be moved to oxide-auth-async even. Don't worry about that latter part in this PR though, that would be much harder to review.

Comment on lines +62 to +64
let default_query_uri = uri!("?b");
let default_query = default_query_uri.query().unwrap();
let query = request.uri().query().unwrap_or(default_query);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need some justification/explanation as in additional comments. Why this specific query string instead of the empty one previously? Is there some type change that requires this—in a first look it seems that maybe a simple clone &query.to_string() would have sufficed? Justifiy the unwraps as well, please.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try more tomorrow, there have been changes to uri/query that broke the previous code.

// Nothing to do if we already have a body, or already generated an error. This includes
// the case where the content type does not indicate a form, as the error is silent until a
// body is explicitely requested.
if let Ok(None) = self.body {
match serde_urlencoded::from_reader(data.open()) {
let data = data.open(2.mebibytes()).into_string().await;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary limit should be a constant somewhere, or, even better, be configurable on the object for the caller. Probably both where a default is set with a constant and it can be overwritten.

@HeroicKatora
Copy link
Owner

Since 4 months ago, rocket = 0.5 has been released. I'd look into a rebase at some point, thanks for the sketches of the expected changes with the pre-releases.

@Maweypeyyu
Copy link
Author

Work took over and I hadn't had a chance to look into this since then. Sorry about that.

@HeroicKatora
Copy link
Owner

Please don't worry about it. Same for me, and rocket dragged that out quite a bit as well. It's FOSS because we appreciate what you do, however far it goes 😄

@Maweypeyyu Maweypeyyu closed this by deleting the head repository Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants