Skip to content

Add OperationId and Tags to identify and group operations, respectively #1237

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
Closed
wants to merge 2 commits into from
Closed

Add OperationId and Tags to identify and group operations, respectively #1237

wants to merge 2 commits into from

Conversation

Acentelles
Copy link

@Acentelles Acentelles commented Nov 4, 2019 •
edited by dmjio
Loading

Documentation for endpoints in the type declaration:

  • OperationId is an optional unique string used to identify an operation. If provided, these IDs must be unique among all operations described in your API.
  • Tags are used to group operations logically by resources or any other qualifier.

Both OperationId and Tags are needed for different HasSwagger instances:

instance (KnownSymbol desc, HasSwagger api) => HasSwagger (OperationId desc :> api) where
  toSwagger _ =
    Servant.toSwagger (Proxy :: Proxy api)
      & Swagger.allOperations . Swagger.operationId ?~ Text.pack (symbolVal (Proxy :: Proxy desc))

instance (SymbolVals tags, HasSwagger api) => HasSwagger (Tags tags :> api) where
  toSwagger _ =
    Servant.toSwagger (Proxy :: Proxy api)
      & Swagger.allOperations . Swagger.tags .~ (Set.fromList $ Text.pack <$> symbolVals (Proxy :: Proxy tags))

@phadej
Copy link
Contributor

phadej commented Dec 14, 2019

This is IMHO too specific, and I cannot see why those should be in servant.

@alpmestan opinions?

@phadej phadej requested a review from alpmestan December 14, 2019 20:24
@Acentelles
Copy link
Author

In the same way paths may have an optional short summary and a longer description for documentation purposes, they may also have an operation id and tags associated (see https://swagger.io/docs/specification/paths-and-operations/). Otherwise, some documentation tools don't display documentation nicely.

Servant already has Description and Summary.

@alpmestan
Copy link
Contributor

alpmestan commented Dec 19, 2019 •
edited
Loading

I'd be tempted to say "why not". (it's a bit hard to draw a line that delimits things that we want in the core packages and things that should live in satellite packages. But in this case, description/summary seem close to operationid/tags in spirit.)

alpmestan
alpmestan requested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@alpmestan alpmestan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against including these combinators. But if we decide to land this, we will need to include those in the comprehensive API that we use for our tests, and make sure to include a test or two to guarantee that this doesn't break in the future. A mention or two somewhere in our docs would be great as well. :-)

@centromere
Copy link

tempted to say "why not"

What are the consequences of being wrong?

@phadej
Copy link
Contributor

phadej commented Dec 19, 2019 via email

@devinlyons devinlyons mentioned this pull request Mar 4, 2020
Closed
@sorki
Copy link
Contributor

sorki commented Jul 31, 2021

There is a similar Tag in servant-util along with couple of instances. Also standalone OperationId PR at #1378

@zerolink-io zerolink-io closed this by deleting the head repository Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alpmestan alpmestan alpmestan requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Acentelles @phadej @alpmestan @centromere @sorki @zerolink-io