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 OperationId combinator. #1277

Closed

Conversation

devinlyons
Copy link

No description provided.

@phadej
Copy link
Contributor

phadej commented Mar 4, 2020

I don't think this belongs to servant. A separate package maybe.

cc @alpmestan

@alpmestan
Copy link
Contributor

alpmestan commented Mar 4, 2020

I have to admit I'm a little bit "bothered" the swagger-specific nature of this combinator. Description/Summary seem general enough to be useful for all sorts of docs-oriented interpretations of API types, while "operation id" seems to be harder to motivate if the only use case lives in the -swagger pkgs.

Any chance this could be useful elsewhere than -swagger?

@devinlyons
Copy link
Author

I don't think it will have any use beyond swagger. Perhaps I can move it to the servant-swagger library. However, that would add a servant-swagger dependency to any servant-client based library that uses the OperationId in the routes. The last option is to create new packages I guess. Thoughts?

@devinlyons
Copy link
Author

I see this is a duplicate of #1237. Perhaps I should just close this and let the existing PR deal with the issue.

@alpmestan
Copy link
Contributor

I think this definitely makes sense as its own little lightweight package. I have myself published a couple of such packages, to avoid "bloating" the core libs with deps and code that only a tiny fraction of users might ever care about (servant-ede, servant-generate, ...).

@roberth
Copy link
Contributor

roberth commented May 4, 2024

I think this definitely makes sense as its own little lightweight package.

It could be done, but the problem with custom combinators is that all packages that interact with servant apis have to support it.
So it wouldn't be just servant-operation-id, but also servant-server-operation-id (with a no-op instance), servant-client-core-operation-id, etc, etc.
(This is an instance of the expression problem.)

By adding OperationId (or something more generic, which I guess is preferable), you make this overhead go away. Each existent package can implement their instances for it cheaply, without incurring an extra dependency.

IMO Hackage, Cabal and GHC should holistically solve the expression problem (of orphan instances), but I can't tell whether that would be within the ambitions of the ecosystem.

@tchoutri
Copy link
Contributor

tchoutri commented May 4, 2024

@roberth Do you reckon this combinator should rather live in servant-openapi3?

@roberth
Copy link
Contributor

roberth commented May 6, 2024

I don't, because then we'd have to convince all the other libraries to add the package for the sole reason of importing OperationId and ignoring it. Seems like a tough sell, especially if the package defines more than just the combinator.
A separate servant-operation-id would be better than adding it to servant-openapi3, but I think it's over-engineered compared to just adding it to servant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants