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: plan media type #2828

Merged
merged 1 commit into from
Jun 18, 2023
Merged

Conversation

steve-chavez
Copy link
Member

@steve-chavez steve-chavez commented Jun 18, 2023

Problem

Previously it was impossible to have doctests for the MediaType module due to recursion:

data MediaType
= MTApplicationJSON
| MTSingularJSON
| MTGeoJSON
| MTTextCSV
| MTTextPlain
| MTTextXML
| MTOpenAPI
| MTUrlEncoded
| MTOctetStream
| MTAny
| MTOther ByteString
| MTPlan MTPlanAttrs
deriving Eq
data MTPlanAttrs = MTPlanAttrs (Maybe MediaType) MTPlanFormat [MTPlanOption]

Basically a MediaType was able to contain another MediaType. See the MTPlan constructor.

This caused the deriving instance Show ... setup to fail.

Solution

Eliminate recursion by having non-plan media types as a separate type(NormalMedia).

Some doctests are now added for MediaType.

Comment on lines +118 to +119
-- >>> decodeMediaType "application/vnd.pgrst.plan;for=\"application/json\""
-- MTPlan (MTPlanAttrs (Just MTApplicationJSON) PlanText [])
Copy link
Member Author

@steve-chavez steve-chavez Jun 18, 2023

Choose a reason for hiding this comment

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

Here I tried adding a space like so:

Suggested change
-- >>> decodeMediaType "application/vnd.pgrst.plan;for=\"application/json\""
-- MTPlan (MTPlanAttrs (Just MTApplicationJSON) PlanText [])
-- >>> decodeMediaType "application/vnd.pgrst.plan; for=\"application/json\""
-- MTPlan (MTPlanAttrs (Just MTApplicationJSON) PlanText [])

And the decoding failed to get Just MTApplicationJSON (got Nothing instead). Although this clearly works as proven on the feature specs:

r <- request methodGet "/projects" (acceptHdrs "application/vnd.pgrst.plan+json; for=\"application/json\"; options=verbose") ""

I thought there must be something wrong with whitespace on the doctests. This would have been easy to prove with the repl. But #2627 continues to be a problem.

@steve-chavez steve-chavez merged commit 3cd3a3f into PostgREST:main Jun 18, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant