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

feat!: add ContentType, expanded Accept parsing #6

Merged
merged 2 commits into from
Sep 8, 2023

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Sep 7, 2023

BREAKING CHANGES

  • ContentType{} to describe a full content type descriptor
  • ParseAccept() returns a slice of acceptable ContentTypes found, ordered by quality factor
  • CheckFormat() returns a single, preferred ContentType
  • Remove spaces from default ContentType formatting
  • Deprecate ResponseContentTypeHeader
  • Deprecate RequestAcceptHeader

BREAKING CHANGES

* ContentType{} to describe a full content type descriptor
* ParseAccept() returns a slice of acceptable ContentTypes found, ordered by
  quality factor
* ParseContentType() returns (ContentType, bool)
* CheckFormat() returns a single, preferred ContentType
* Remove spaces from default ContentType formatting
* Deprecate ResponseContentTypeHeader
* Deprecate RequestAcceptHeader
@rvagg rvagg force-pushed the rvagg/more-accept-parsing branch 2 times, most recently from d9f0719 to 83d1e83 Compare September 7, 2023 05:52
DefaultOrder = ContentTypeOrderDfs // The default value for an unspecified "order" parameter.

ContentTypeOrderDfs ContentTypeOrder = "dfs"
ContentTypeOrderUnk ContentTypeOrder = "unk"
Copy link
Member

Choose a reason for hiding this comment

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

is this a useful value? we don't seem to do anything with an unknown ordering, and if the ordering is anything other than an understood 'dfs' we presumably would want to stream it through unchanged rather than try to change it to "unk"? since we'd be comparing against known values, like 'dfs', what do we get by defining and parsing an 'unk' value here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's defined by the spec https://specs.ipfs.tech/http-gateways/trustless-gateway/#car-order-content-type-parameter

I'm not particularly a fan of this existing but but the idea of it being in here is that a consumer of this library can decide what to do with it. Currently Lassie will ignore it and assume dfs and error on you for feeding bad data. Frisbii will always give you dfs regardless of what you ask for (on the assumption that dfs is a subset of unk so this is acceptable).

This isn't new btw, it was in Lassie like this; no behaviour should be changing with this, it just gets surfaced as a parameter you can inspect now if you care.

Copy link
Member

Choose a reason for hiding this comment

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

it does seem like premature complexity / over engineering

Copy link
Member Author

Choose a reason for hiding this comment

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

ipfs/specs#412

It was nearly dfs, rnd, weak!

Our approach atm is to 🙈; although it's at least exposed here if you actually wanted to do something with it. I'm going to assume that nobody will that wants to interoperate with our current tools (Lassie primarily).

@rvagg rvagg merged commit e2c362f into master Sep 8, 2023
9 checks passed
@rvagg rvagg deleted the rvagg/more-accept-parsing branch September 8, 2023 03:55
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.

2 participants