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

Add LenientData #45

Merged
merged 1 commit into from Jan 19, 2017

Conversation

Projects
None yet
2 participants
@phadej
Copy link
Collaborator

commented Jan 18, 2017

Motivated by servant making parameter parsing fail.

Is it ok to define ToHttpApiData instance with toQueryParam = either (const "") toQueryParam . getLenientData, i.e. serialising Left to an empty string?

@fizruk

This comment has been minimized.

Copy link
Owner

commented Jan 18, 2017

Ok, what options do we have here? So far I see these:

  1. toQueryParam = either (const "") toQueryParam . getLenientData.
  2. toQueryParam = either (error "Can't encode") toQueryParam . getLenientData.
  3. No ToHttpApiData instance.

I like option 3 the most. Because LenientData makes sense only for decoding. However this means that we won't be able to use client with QueryParam "param" (LenientData MyParam).

Option 2 is bad because of error and option 1 is bad because it silently ignores Left (which may or may not lead to problems in real code).

What I would do is rather use Lenient as a parameter of QueryParam (much like Required). So that we could have

type API = LenientQueryParam "param" MyParam

This would allow us to have Maybe (LenientData MyParam) in handler while using just Maybe MyParam on the client side. If combined with Required this might even be

type API = RequiredLenientQueryParam "param" MyParam

Which would result in LenientData MyParam on server and MyParam on client.

If Servant were to implement QueryParam (and other combinators) with Lenient option we won't have to ever use ToHttpApiData instance for LenientData a since on client we will always have just a.

@phadej

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2017

I'm 👍 with your plan, so the QueryParam will be parametrised by requiredness and leniency of parsing. That makes sense

@fizruk

This comment has been minimized.

Copy link
Owner

commented Jan 18, 2017

I'm otherwise ok with this PR, are you going to add anything else here or should I merge it already?

@phadej

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2017

I'll add few instances, Foldable and Traversable could be useful

@phadej phadej force-pushed the phadej:lenient-param branch from 272a2d3 to 64e6fce Jan 19, 2017

@phadej

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2017

@fizruk,I'd say this is ready. Even we won't need ToHttpApiData instance, theres no downside of having it.

foldMap f (LenientData (Right x)) = f x

instance Traversable LenientData where
traverse f (LenientData x) = fmap LenientData (traverse f x)

This comment has been minimized.

Copy link
@fizruk

fizruk Jan 19, 2017

Owner

Why don't you just derive Functor, Foldable and Traversable?

This comment has been minimized.

Copy link
@phadej

phadej Jan 19, 2017

Author Collaborator

autoderivied all of them


-- | Lenient parameters. 'FromHttpApiData' combinators always return `Right`.
newtype LenientData a = LenientData { getLenientData :: Either Text a }
deriving (Eq, Ord, Show, Read, Typeable)

This comment has been minimized.

Copy link
@fizruk

fizruk Jan 19, 2017

Owner

Why don't you also derive Data?

@phadej phadej force-pushed the phadej:lenient-param branch from 64e6fce to a5f4e27 Jan 19, 2017

@fizruk

This comment has been minimized.

Copy link
Owner

commented Jan 19, 2017

Nice, thanks!

@fizruk fizruk merged commit 883b997 into fizruk:master Jan 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@phadej phadej deleted the phadej:lenient-param branch Feb 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.