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.

Sign up for GitHub

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

Jump to bottom

Servant.Links.addQueryParam not exported? #1232

Open
saurabhnanda opened this issue Oct 1, 2019 · 7 comments · May be fixed by #1785
Open

Servant.Links.addQueryParam not exported? #1232

saurabhnanda opened this issue Oct 1, 2019 · 7 comments · May be fixed by #1785
Labels
enhancement help wanted

Comments

@saurabhnanda
Copy link

saurabhnanda commented Oct 1, 2019 •
edited
Loading

I was trying to write a ToLink instance for a custom combinator called AllQueryParams (that captures all query parameters). Unfortunately, it seems almost impossible without resorting to type-level magic, because neither are the constructors of Link exported, nor is addQueryParams exported.

Is there an obvious way of doing this, which I might be missing?

Here's my (non-working) code snippet:

data AllQueryParams

instance (HasServer api context) => HasServer (AllQueryParams :> api) context where
  type ServerT (AllQueryParams :> api) m = ([(BS.ByteString, Maybe BS.ByteString)] -> ServerT api m)

  hoistServerWithContext _ pc nt s = hoistServerWithContext (Proxy :: Proxy api) pc nt . s

  route _ context subServer = route (Proxy :: Proxy api) context delayed
    where
      delayed = passToServer subServer queryString

instance (HasLink sub) => HasLink (AllQueryParams :> sub) where
  type MkLink (AllQueryParams :> sub) a = ([(BS.ByteString, Maybe BS.ByteString)] -> MkLink sub a)

  toLink toL _ lnk qparams = toLink toL (Proxy :: Proxy sub) $
                             DL.foldl' (\l (k, v) -> addQueryParam (SingleParam (toS k) (toS $ fromMaybe "" v)) l) lnk qparams
@phadej
Copy link
Contributor

phadej commented Oct 1, 2019 •
edited
Loading

I don't see a reason for it not to be exported. Please make a PR.

@saurabhnanda
Copy link
Author

Any chance that it could make the concept of safe links unsafe?

@phadej
Copy link
Contributor

phadej commented Oct 1, 2019 •
edited
Loading

There's a balance between being extensible, vs. preventing from programming errors. Maybe addSegment and addQueryParam should be exported from Servant.Links.Internal

@alpmestan opinions?

@alpmestan
Copy link
Contributor

We've usually tried to keep things extensible whenever/wherever appropriate, and I think we definitely want to allow users to hook their own combinators in the link making machinery.

I don't think that really makes our links less type safe. The only way to get them wrong is to make a mistake in an instance or do something bad with them, explicitly, after the link making function handed us correct ones. In the first case, well, it's easy to spot with some tests, and the code for those instances is fairly easy to follow. In the second, well it should be obvious where the problem comes from.

Finally, if people have to get those functions from Servant.Links.Internal, then they know they're exposed to the guts and have to be careful not to introduce errors themselves, I think.

=> I'm all for exporting those utilities from somewhere, especially if that somewhere makes it obvious that it's not the public API that users who just want to produce links need.

@poscat0x04
Copy link

I just bumped into this problem as well. IMO HasLink is too restrictive right now.

@alpmestan
Copy link
Contributor

A PR is still welcome :-)

@saurabhnanda
Copy link
Author

I hit this again, while solving #1784 for my own codebase... I think I'll raise a PR now if no one has any objections 😄

@saurabhnanda saurabhnanda linked a pull request Sep 23, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Exported Servant.Links.addQueryParam saurabhnanda/servant
4 participants
@saurabhnanda @phadej @alpmestan @poscat0x04