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

Prefer exact match without parameters to one with extra parameters #36

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

suola
Copy link

@suola suola commented Oct 14, 2018

If 'type/subtype' and 'type/subtype;param=1' are supported and
'type/subtype' is accepted, prefer 'type/subtype'.

If 'type/subtype' and 'type/subtype;param=1' are supported and
'type/subtype' is accepted, prefer 'type/subtype'.
@suola suola changed the title Prefer exact without parameters to one with extra parameters Prefer exact match without parameters to one with extra parameters Oct 14, 2018
@vytas7
Copy link
Member

vytas7 commented Oct 26, 2021

Hi @suola , and thanks for this PR!
Now that maintenance of this package has been moved to Falconry, we'll take a new look at this.

@vytas7 vytas7 requested a review from CaselIT October 26, 2021 12:04
@CaselIT
Copy link
Member

CaselIT commented Oct 26, 2021

I'm not sure this is best way of doing this, since it returns different results when using q values.
Like with application/json with the options ["application/json;q=0.2", "application/json;q=0.6;foo=bar"] it returns application/json;q=0.2 while it should return application/json;q=0.6;foo=bar

mimeparse.py Outdated
# 1 bonus point if neither has any parameters
if (set(target_params) | set(params)) - {'q'} == set():
fitness += 1

# finally, add the target's "q" param (between 0 and 1)
fitness += float(target_params.get('q', 1))
Copy link
Member

Choose a reason for hiding this comment

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

as currently implemented the q has basically no say

Copy link
Author

Choose a reason for hiding this comment

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

That's true, I fixed this + added a test case for that

suola added 2 commits October 28, 2021 09:54
Take q into account also when comparing options with same type but different
parameters.

Change fitness into a list of metrics in decreasing order of precedence. This
cleaner than building a floating point value now that a condition is evaluated
after `q`.
Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

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

It makes sense to me.

I'm not sure regarding the change in api change, but I think it would be ok since I think we plan on releasing v2 so api changes would be ok

@CaselIT CaselIT requested a review from vytas7 November 1, 2021 21:51
@vytas7
Copy link
Member

vytas7 commented Nov 10, 2024

We need to rebase this PR now that we moved mimeparse.py into a module...

Speaking of the changes in general, maybe we could just leave the current behavior as-is?
OTOH, it seems the current algorithm has other related issues, see #53.

@suola btw if you were interested in this in the context of Falcon, this has been fixed by rolling out a different implementation there, see falcon.mediatypes.quality().

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