Skip to content
This repository has been archived by the owner on Jun 21, 2020. It is now read-only.

Adds support for q-values, some refactoring #1

Merged
merged 6 commits into from
May 10, 2019

Conversation

fairingrey
Copy link
Contributor

@fairingrey fairingrey commented May 9, 2019

Changes

This PR does a number of things:

Checklist

  • tests pass
  • tests and/or benchmarks are included
  • documentation is changed or added

Context

This seeks to extract some of the Accept-Encoding parsing logic in http-rs/tide#194 to this crate such that it becomes more modular and easier to test and reason about. Not only that, but other developers might also use this crate for their own needs; it helps if it supports the major use cases in parsing Accept-Encoding header by parsing q-values.

Semver Changes

Increment the minor version (0.1.0 -> 0.2.0) as there are breaking changes.

adds support for q-values per spec at https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#Directives
adds extra values and potentially helpful functions to encoding
new tests
changes rustfmt.toml to look similar to Tide's
src/lib.rs Outdated Show resolved Hide resolved
@fairingrey
Copy link
Contributor Author

fairingrey commented May 10, 2019

Also, someone came to me with the idea that I should also probably expose an iterator for accept-encoding, just in case they want to be responsible for deciding which encoding to use.

In practice it would be a struct Encodings that would impl the Iterator trait, returning items of (Encoding, f32), where the latter is the q-val and takes headers HeaderMap as an inner argument. I think I'll be opening another PR for that later, however.

EDIT: It ended up being a vector, which I think is easier to work with.

Copy link
Member

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This is excellent; thanks so much!

@yoshuawuyts yoshuawuyts merged commit 6303023 into http-rs:master May 10, 2019
@fairingrey fairingrey deleted the refactor branch May 10, 2019 12:03
@@ -12,8 +12,8 @@ readme = "README.md"
edition = "2018"

[dependencies]
derive_is_enum_variant = "0.1.1"
failure = "0.1.3"

Choose a reason for hiding this comment

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

@fairingrey Should we be injecting failure as a dependency for library? If it is used, other users need to pull it as well. IIRC most libraries does not use failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, you'll have to ask @yoshuawuyts. It was already present when I started working on this PR 😅

I would probably remark that there might be a better way to do error handling, but I don't have any strong opinions on it. FWIW, per failure's best practices we do implement Fail for our Error type so it's easier to work with: https://docs.rs/failure/0.1.5/failure/

@fairingrey fairingrey mentioned this pull request May 11, 2019
3 tasks
if (qval - 1.0f32).abs() < 0.01 {
preferred_encoding = encoding;
break;
} else if qval > 1.0 {

Choose a reason for hiding this comment

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

Should we move this branch as the last branch?

I think error handling branches can come at last.

Copy link
Contributor Author

@fairingrey fairingrey May 11, 2019

Choose a reason for hiding this comment

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

It's a little bit difficult to track the comments here since this PR was merged already, but I'll reply here anyways.

Actually, there is a slip of logic here that would require the predicate there to actually come first instead of last (since qval can technically be 1.001 and it would still pass, even though ideally it should fail since a q-value shouldn't be greater than 1.0 flat).

This is actually addressed indirectly through the refactoring on the second PR I've made: #2

Since the logic in that function runs first anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants