Skip to content

#23 Handle HEAD requests #31

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

Merged
merged 9 commits into from
Nov 13, 2018
Merged

#23 Handle HEAD requests #31

merged 9 commits into from
Nov 13, 2018

Conversation

DeltaManiac
Copy link
Contributor

@DeltaManiac DeltaManiac commented Nov 8, 2018

@aturon

Have I missed something crucial here?

Analyzing with wireshark I do observe that there is no body being sent in the response just the headers when triggering a HEAD invocation

@aturon
Copy link
Collaborator

aturon commented Nov 8, 2018

Thanks @DeltaManiac!

So, the goal with #27 is to be able to write a GET endpoint and automatically get a HEAD endpoint, ergonomically, while still providing a way to override the behavior of HEAD if you want.

@DeltaManiac
Copy link
Contributor Author

Ok, then this is definitely not going to work, thanks for the clarification

@DeltaManiac DeltaManiac changed the title Closes #27 #27 Handle HEAD requests Nov 9, 2018
@DeltaManiac
Copy link
Contributor Author

HI @aturon

I've made some changes to automatically fallback to the GET implementation if there is not specialized implementation for HEAD while still allowing users to override this behavior if necessary.

Please let me know if this is ok or there are anymore changes you'd like

@DeltaManiac DeltaManiac changed the title #27 Handle HEAD requests #23 Handle HEAD requests Nov 9, 2018
@aturon
Copy link
Collaborator

aturon commented Nov 9, 2018

This is heading in the right direction! But can you pull out the call to route to before the conditional logic, so that it doesn't have to be repeatedly called?

@DeltaManiac
Copy link
Contributor Author

Done

src/router.rs Outdated
self.table
.route(path)
.and_then(|(r, p)| Some((r.endpoints.get(method)?, p)))
let route = self.table.route(path).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This unwrap will panic if the route isn't found. You should be able to replace it with route(path)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Sorry about overlooking that the first time around :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries!

src/router.rs Outdated
self.table
.route(path)
.and_then(|(r, p)| Some((r.endpoints.get(method)?, p)))
let route = self.table.route(path)?;
Copy link
Contributor

@liufuyang liufuyang Nov 12, 2018

Choose a reason for hiding this comment

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

Maybe do something like let (route, route_match) = self.table.route(path)?; here? (I am not sure what could be the best names here though)

Then later you don't have to use route.0 and route.1 below?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, that'd be a nice code cleanup. Other than that, looks good to go!

Copy link
Contributor Author

@DeltaManiac DeltaManiac Nov 13, 2018

Choose a reason for hiding this comment

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

I agree this enhances readability and I've done the changes,

Just for a personal learning, is using the tuple.0,tuple.1 syntax generally avoided?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am new to Rust as well, might not have the best opinion here but I simply find tuple.0, tuple.1 difficult to read as they look like the tuple type but it could be completely a different type. Just from tuple.0 I don't know the name of it and I don't know the type of it, without looking for the definition of tuple somewhere else. I guess if you use them only once or twice and they are very close to where tuple is defined/created, then maybe it is fine. Especially if you only have one element in the tuple (like a wrap of some other type) then using tuple.0 around is probably okay. Otherwise give them names might help a bit in most cases. Just my opinion :P

@aturon aturon merged commit 0894aa1 into http-rs:master Nov 13, 2018
@aturon aturon mentioned this pull request Nov 13, 2018
@aturon
Copy link
Collaborator

aturon commented Nov 13, 2018

Merged, thanks for the contribution!

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.

3 participants