Simplify terminology and behavior.#4
Conversation
- Rename parseMediaRange to parseAndNormalizeMimeType to better explain its relationship with parseMimeType. - Update parseAndNormalizeMimeType to not eat parseMimeType's generic subtype. - Update documentation and tests to reflect changes.
- Rename parseMimeType and parseAndNormalizeMimeType to *MediaRange. - Rename several variables in the same fashion. - Update documentation where appropriate. - Update tests to reflect changes (also fix a stray usage of "format" for "generic subtype").
|
Most of the documentation and terminology all comes from the original code here: http://code.google.com/p/mimeparse/ I simply copied it over mostly verbatim. I like and agree with your changes, but I'll have to give it a little thought, since it does break backwards compatibility. In addition, your pull request removes code that I've since added 4-5 days ago, so you'll need to pull the latest from master before I can accept this request. Thanks! |
|
I forked after your latest commit, 8c53c4f. If you check out the network graph you should see that my repository is up to date. Let me know if there's any changes I should make or anything else I can do to make things easier. |
There was a problem hiding this comment.
Did you choose to remove the $tieBreaker parameter? If so, why?
There was a problem hiding this comment.
Yes. As stated in my commit messages I changed the behavior to use the order of the $supported array to break ties, making $tieBreaker unnecessary.
This allows for more flexible behavior which would not be possible with a single $tieBreaker parameter, including having multiple fallbacks (this is especially helpful when the specified $tieBreaker is not among the best matches, but there is still a tie) and easier handling of tie breakers when the $supported array is not constant.
It also specifies previously-unspecified behavior: What happens when there is a tie but no $tieBreaker (or one which does not match)? Now that condition is impossible.
There was a problem hiding this comment.
Oops. I glossed over that when originally reading the PR description. Sorry about that. I like your approach better. I had given that approach some thought when I built in the $tieBreaker, but my original goal was to stick as close to the original implementation as possible, and changing the behavior of $supported breaks what is expected in the other implementations (i.e. Java, Python, Ruby, etc.).
Given that your updates change the naming and behavior expected in all the other implementations, I'm less inclined to follow their convention, and I'm okay with merging in your changes.
There was a problem hiding this comment.
Thankfully, making the order of $supported significant only serves to specify unspecified behavior in the other implementations (it doesn't actually "change the rules", it just "makes a new rule"). I haven't tested other implementations' behavior as far as what happens when there are ties, but I wouldn't be surprised to learn that there are already inconsistencies between them.
Thanks for reviewing/merging, and again sorry that I handed this over to you as one big fat blob of changes. In the future I'll try to submit more atomic pull requests. 😄
Simplify terminology and behavior.
2 participants
$supportedarray (mime-types closer to the beginning of the array are preferred) instead of a separate$tieBreakerparameter.The public-facing (possibly backwards-incompatible) changes are as follows:
parseMimeType()becomesparseMediaRange().UnexpectedValueExceptionthrown byparseMediaRange()is different.bestMatch()loses its$tieBreakerparameter, and the order of the$supportedarray becomes significant.Apologies for the large changeset. I can try to split this up into multiple pull requests if you'd prefer, but it may take some time. Hopefully my commit history is clean enough for you to easily review.