search

jshttp / negotiator

An HTTP content negotiator for Node.js
MIT License
309 stars 33 forks source link

Remove checks for the same specificity of different accept entries #25

Closed jdeerhake closed 10 years ago

jdeerhake commented 10 years ago

As far as I can tell, the only case where this conditional (a.s == b.s) is true is where there are multiple entries in the accept header that have the same type, subtype, and params. This scenario isn't mentioned in the RFC and I can't think of a real world use case for it.

Removing these helps #24 by getting line coverage to 100%.

Fishrock123 commented 10 years ago

These variables need better names.

dougwilson commented 10 years ago

Yea, but I need to look over this module :) I have this PR pending because it removes something, but only in the name of code coverage, so I cannot be sure it can actually be removed (though my short look earlier seems like it can be).

federomero commented 10 years ago

I've been thinking about this and there is an edge case where this check is needed. Consider we have the following Accept header:

Accept: application/json;q=0.9, text/html;q=0.8, application/json;q=0.7

Now, as you can see application/json appears twice. Even though this header is probably not valid, a user may still supply it and I guess it would make sense to consider the highest q for application/json.

federomero commented 10 years ago

Regarding variable names, I think a and b are valid variable names considering it's a sort function.

The properties q and s could probably have more descriptive names. They stand for quality and specificity I guess.. I'm not sure renaming q to quality is a good idea though considering that it appears as q in the headers.

dougwilson commented 10 years ago

Even though this header is probably not valid

Yes, though nothing that I know of in the RFCs specifically say you cannot send that header, so it could certainly occur and there's no reason not to support it, weird as it may be :)

jdeerhake commented 10 years ago

Is there anything that actually supplies a header like that? This change doesn't really remove support for it (nothing will blow up with that header) it just changes the behavior slightly.

Given that the RFC makes no mention of what should happen and there's no obvious real world application for it (that I can think of?) I think we would want go with the simplest thing possible.

dougwilson commented 10 years ago

it just changes the behavior slightly

can you explain how the behavior changes for the header posted?

Given that the RFC makes no mention of what should happen and there's no obvious real world application for it (that I can think of?) I think we would want go with the simplest thing possible.

no, that's not how making things people rely on works :) the RFC (which is RFC 7231, btw) clearly specifies the behavior makes no special case for if you supply the same type more than once. if your change does not keep one instance at the 0.9 position and one at the 0.7 position, i don't think it fits in with RFC 7231.

dougwilson commented 10 years ago

also, you're not actually helping #24 by changing the behavior of the module to get there :) it's for covering the code that exists already by adding more tests, not reducing the code to make the existing tests cover more of it.

dougwilson commented 10 years ago

btw, i closed this because your comment made it sound like you are changing the behavior of this module just to increase code coverage, which make no sense at all. one of the reasons we need to increase the code coverage is to know what the consequences of changes to the code are and if a change would necessitate a major version bump, etc.