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

Refactor ContentType + improve content type matching #173

Merged
merged 6 commits into from
Aug 8, 2023

Conversation

czechboy0
Copy link
Contributor

Motivation

The ContentType type had two issues:

  • the enum where all cases had the same associated string value was not a good representation of the data
  • the parsing was too strict and didn't match e.g. foo/bar+json as a JSON content type

Modifications

Refactored ContentType to split the raw value storage from the Category (new term, ideas of a better name are welcome) of content types (json, text, or binary).

Also, improved the mapping to match https://json-schema.org/draft/2020-12/json-schema-core.html#section-4.2

Result

Now the type is easier to work with, and parsing correctly recognizes foo/bar+json as JSON.

Test Plan

Added unit tests.

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good but could you add a comment to the binary case before merging?

@czechboy0 czechboy0 requested a review from glbrntt August 8, 2023 07:20
@czechboy0
Copy link
Contributor Author

@glbrntt, actually I made a few more changes in 91828ac, could you take a look if that still has your approval? It was aimed at avoiding callers accidentally using case sensitive comparisons of content types, plus it allows access to the parsed type and subtype components.

@czechboy0 czechboy0 merged commit fd64063 into apple:main Aug 8, 2023
@czechboy0 czechboy0 deleted the hd-refactor-content-type branch August 8, 2023 07:33
@czechboy0 czechboy0 added the semver/none No version bump required. label Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants