-
-
Notifications
You must be signed in to change notification settings - Fork 413
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 missing IsElem instance for NamedRoutes #1699
Conversation
The CI build failure seems unrelated to my change and exactly the same as the one in #1698. |
This looks good but it is missing a changelog entry. See the other PRs for how to make one. |
6e360a1
to
529cf90
Compare
Added a changelog entry, any thoughts on where the |
I think that people will only appreciate them if they see examples. And if they see examples, it almost doesn't matter what the name of the combinator is. Personally, I think it is unnecessary since their definitions are trivial. There is no reason to abstract away function application. |
While |
529cf90
to
12033e7
Compare
You could submit a separate pull request for that, for now I will merge this since it fixes CI and the instance is useful even without the combinators. |
Added various tests for NamedRoutes too.
In writing the tests I realised that the client combinators
//
and/:
can be used to create links in the same natural way.I've added the combinators to the test suite for now, but it probably makes sense to put them somewhere in the
servant
package, though I wasn't sure where. Maybe just in the rootAPI
module, and then ensure thatHasClient
andReexport
inservant-client-core
reexport them for any backwards compatibility?Fixes #1674.