Skip to content

Implement custom q value parser according to spec#10

Merged
maxcountryman merged 1 commit intomaxcountryman:mainfrom
jannschu:qvalue-parser
Jul 11, 2025
Merged

Implement custom q value parser according to spec#10
maxcountryman merged 1 commit intomaxcountryman:mainfrom
jannschu:qvalue-parser

Conversation

@jannschu
Copy link
Copy Markdown
Contributor

No description provided.

@maxcountryman maxcountryman requested a review from Copilot July 11, 2025 13:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Implements a new QValue type to parse and compare HTTP quality values according to the RFC specification, replacing the previous f32-based logic.

  • Replaces f32 quality fields with QValue and updates comparisons in best_match and sorting routines.
  • Adds a QValue struct with FromStr, Ord, and utility methods plus comprehensive unit tests.
  • Refactors parse_q_value to return QValue with unwrap_or_default() for missing parameters.
Comments suppressed due to low confidence (3)

src/lib.rs:156

  • Add a unit test for Accept::best_match or equivalent that verifies media types with q=0 are correctly excluded by this branch.
                    if quality.is_zero() {

src/lib.rs:215

  • Introduce a test that confirms media types are sorted by specificity and then by QValue, to ensure the new sort_by_key behavior matches expectations.
        media_types.sort_by_key(|x| {

src/lib.rs:224

  • [nitpick] The method name parse_q_value now returns a QValue type instead of f32; consider renaming it to parse_qvalue or updating its doc comment to reflect the new return type more clearly.
    fn parse_q_value(media_type: &MediaTypeBuf) -> QValue {

Comment on lines +385 to +402
fn parse_fractional(digits: &[u8]) -> Result<u16, HeaderError> {
digits
.iter()
.try_fold(0u16, |acc, &c| {
if c.is_ascii_digit() {
Some(acc * 10 + (c - b'0') as u16)
} else {
None
}
})
.map(|num| match digits.len() {
1 => num * 100,
2 => num * 10,
_ => num,
})
.ok_or_else(HeaderError::invalid)
}

Copy link

Copilot AI Jul 11, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting the nested parse_fractional function out of the FromStr impl into a private helper at module scope for clarity and easier unit testing.

Suggested change
fn parse_fractional(digits: &[u8]) -> Result<u16, HeaderError> {
digits
.iter()
.try_fold(0u16, |acc, &c| {
if c.is_ascii_digit() {
Some(acc * 10 + (c - b'0') as u16)
} else {
None
}
})
.map(|num| match digits.len() {
1 => num * 100,
2 => num * 10,
_ => num,
})
.ok_or_else(HeaderError::invalid)
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could also inline the function since it is only called once

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not putting a lot of stock in what the AI says, I'm mostly just curious if it's at all intelligent.

@maxcountryman
Copy link
Copy Markdown
Owner

Thanks for this. We can cut a new release inclusive of this and the last patch. I'll bump the minor version since this is technically breaking and we're not yet 1.0.

@jannschu
Copy link
Copy Markdown
Contributor Author

Sure, thanks!

@maxcountryman maxcountryman merged commit 0e7207c into maxcountryman:main Jul 11, 2025
4 checks passed
@jannschu jannschu deleted the qvalue-parser branch July 18, 2025 16:13
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