search

haskell-servant / servant

Main repository for the servant libraries — DSL for describing, serving, querying, mocking, documenting web applications and more!
https://docs.servant.dev/
1.8k stars 407 forks source link

Add missing IsElem instance for NamedRoutes #1699

Closed RaoulHC closed 11 months ago

RaoulHC commented 12 months ago

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 root API module, and then ensure that HasClient and Reexport in servant-client-core reexport them for any backwards compatibility?

Fixes #1674.

RaoulHC commented 12 months ago

The CI build failure seems unrelated to my change and exactly the same as the one in #1698. Couldn't reproduce it locally, though I wasn't able to get ghc-8.6.5 working on my system, so I've added a constraint to use the previous version for 8.6.5, though I'm not sure this will fix it. Running the same version of cabal locally (3.10.1.0), so it seems odd that it's failing to parse wreq.cabal.

ysangkok commented 11 months ago

This looks good but it is missing a changelog entry. See the other PRs for how to make one.

RaoulHC commented 11 months ago

Added a changelog entry, any thoughts on where the // and /: might belong in servant?

ysangkok commented 11 months ago

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.

RaoulHC commented 11 months ago

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 // is just a redefinition of &, /: does make it more ergonomic imo. Either way, these helpers are already defined in Servant.Client.Core.HasClient, I just think it might sense to move them to the servant package. If we do that then the documentation should be updated to have examples for both links and clients.

ysangkok commented 11 months ago

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.