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

Added recommandation against enums #133

Merged
merged 2 commits into from
Sep 27, 2016
Merged

Conversation

tkrop
Copy link
Member

@tkrop tkrop commented Aug 30, 2016

Fixes #119

@ePaul
Copy link
Member

ePaul commented Aug 30, 2016

Maybe adapt the compatibility section which mentions this, too.

@tkrop
Copy link
Member Author

tkrop commented Aug 30, 2016

Added links and improved wording.

@ePaul
Copy link
Member

ePaul commented Aug 30, 2016

During the meeting we discussed whether the name x-extensible-enum for this property is really the right thing (no decision was yet done about this).

I originally proposed this name for the extension property in the guidelines, and took the syntax from Swagger's enum property (which came from JSON schema). In JSON schema (and thus also in Swagger/OpenAPI) enum restricts the values to just the given values – which is useful for validation, and in code-generating tools is also used for generating enums in programming languages.

Some arguments for/against changing:

  • "Extensible enum" is an oxymoron, and thus a bad name.
  • What we have here is basically a list of examples (for which possibly some meaning is already defined – though not documented), not a list of all possible examples (which would be an enumeration).
  • We already established this name, and it is used in several API definitions already. Changing it now would confuse people.
  • There is no (known) software support for this property name, changing it would not break any API (nor implementation).

@ALL What would be better names? Just x-examples? (Other proposals welcome.)

If we are already changing the name of the property, we could consider to add a description for each item. The next version of OpenAPI likely will have be some way of documenting each enum item, maybe we want to do this similarly? (Though that is not yet finished.)

@whiskeysierra
Copy link
Contributor

@ALL What would be better names? Just x-examples? (Other proposals welcome.)

x-samples, pun intended

@whiskeysierra
Copy link
Contributor

+1 on using an object for this to add descriptions per value.

@tkrop
Copy link
Member Author

tkrop commented Aug 31, 2016

+1 on using an object for this to add descriptions per value.

@whiskeysierra hmm, can you give an example, what you have in mind?

@ePaul
Copy link
Member

ePaul commented Aug 31, 2016

@tkrop
One possible way (which would work not just for strings, also for whole objects or similar):

  Event:
    description:
      An event which will be delivered to Nakadi (or other event sinks),
      together with some metadata used by Tarbela.
    type: object
    properties:
      [...]
      delivery_status:
        type: string
        description: |
          The delivery status of an event.
       x-samples:
        - value: NEW
          description:
             the event was created in the producer, and not yet tried to publish.
        - value: SENT
          description: the event was successfully published to the event sink.
        - value: ERROR
          description: something did go wrong with publishing the event.

For enum-like string properties something like this would also work:

properties:
  delivery_status:
    type: string
    description: the code of the discount type for this discount condition.
       x-samples:
         NEW: the event was created in the producer, and not yet tried to publish.
         SENT: the event was successfully published to the event sink.
         ERROR: something did go wrong with publishing the event.

(This could go into Tarbela's producer API).

@tkrop
Copy link
Member Author

tkrop commented Sep 6, 2016

@ePaul @whiskeysierra great suggestion. Which one should I pick for example? both?

@whiskeysierra
Copy link
Contributor

The first one is more powerful, I'd say. But I don't like to have Java-idomatic enum values (upper underscore). I think snake case or lower hyphen would be a better fit.

@tkrop
Copy link
Member Author

tkrop commented Sep 6, 2016

@whiskeysierra I think it is not only Java, but many other programming languages too ... event if http://blog.josephwilk.net/rhetorical-programming/why-are-you-shouting-programmer.html isn't a good reference ;)

@ALL I'm willing to change it to lower snake case if some more reviewers support this request.

@ePaul
Copy link
Member

ePaul commented Sep 6, 2016

The example was from an existing API, which used upper-case values. For an example in the Guidelines of how this should look of course you can write them differently. (I guess we still need some explanation text around this.)

@whiskeysierra
Copy link
Contributor

👍

1 similar comment
@fmueller
Copy link
Contributor

👍

@fmueller fmueller merged commit 79272bb into master Sep 27, 2016
@fmueller fmueller deleted the feature/recommend-against-enums branch September 27, 2016 08:18
maxim-tschumak added a commit that referenced this pull request Nov 14, 2017
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.

4 participants