Skip to content

Conversation

@krishahn
Copy link
Contributor

@krishahn krishahn commented Jun 7, 2017

Suggested wording of a couple of sentences that made me pause.

versions/3.0.md Outdated
```

While the API is described using JSON, it does not impose a JSON input/output to the API itself.
While the API is described using JSON, input to and output from the API is not required to be JSON.

Choose a reason for hiding this comment

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

This sentence always bothered me because I think it is out of place. I suggest moving it to the end of the section, and perhaps making it stand out as a side note:

Note: While APIs are described with JSON or YAML Open API documents, the APIs' request and response bodies and other content are not required to be JSON or YAML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sgtm, i'll adjust pr

versions/3.0.md Outdated
The schema exposes two types of fields: Fixed fields, which have a declared name, and Patterned fields, which declare a regex pattern for the field name.

Patterned fields can have multiple occurrences as long as each has a unique name.
Multiple occurrences of patterned fields need to have unique names.

Choose a reason for hiding this comment

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

I suggest using MUST and limiting the scope of the restriction. (Since line 114 uses capital Patterned fields, this should also, but my suggestion moves it to the beginning of the sentence.)

Patterned fields MUST have unique names within the containing object.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to @DavidBiesack's comment "need" -> "MUST" seems like a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll adjust PR.

versions/3.0.md Outdated
The files describing the RESTful API in accordance with this specification are represented as JSON objects and conform to the JSON standards.
YAML, being a superset of JSON, can be used as well to represent an OAS file.
The files describing the RESTful API in accordance with this specification consist of JSON objects and conform to the JSON standards. An OAS file written in
YAML, a superset of JSON, also complies with this specification.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if the edit makes it clearer.

What we want to say here:

An API definition following this OpenAPI specification is a JSON object, which can be represented either in JSON or YAML format.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, saying the definition is a JSON object does not allow for the yaml superset features implied, such as comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say any YAML comments are not part of the definition document (just syntactically spread within it – they are ignored by a parser, right?). As far as I am aware, nothing in the specification talks about comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'll revise the PR then to use, "An API definition following this OpenAPI specification is a JSON object, which can be represented either in JSON or YAML format."

Copy link
Member

Choose a reason for hiding this comment

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

May I propose another minor wordsmithing? An API description that conforms to the OpenAPI Specification is itself a JSON object, which may be represented either in JSON or YAML format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed PR

versions/3.0.md Outdated
- Tags MUST be limited to those allowed by the [JSON Schema ruleset](http://www.yaml.org/spec/1.2/spec.html#id2803231).
- Keys used in YAML maps MUST be limited to a scalar string, as defined by the [YAML Failsafe schema ruleset](http://yaml.org/spec/1.2/spec.html#id2802346).

**Note:** While APIs are described with JSON or YAML Open API documents, the API request and response bodies and other content are not required to be JSON or YAML.
Copy link
Member

Choose a reason for hiding this comment

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

Re: described with JSON or YAML Open API documents, may I suggest described by OpenAPI documents in YAML or JSON format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixing ...


The files describing the RESTful API in accordance with this specification consist of JSON objects and conform to the JSON standards. An OAS file written in
YAML, a superset of JSON, also complies with this specification.
An API description that conforms to the OpenAPI Specification is itself a JSON object, which may be represented either in JSON or YAML format.

Choose a reason for hiding this comment

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

If we adopt my suggestion on #1154 then this would be "An OpenAPI definition that conforms to the OpenAPI Specification...." (rather than An API description)

@RobDolinMS
Copy link
Contributor

#TDC: Merging this as-is. If there are additional suggestions for improvements, please submit an additional PR.

@RobDolinMS RobDolinMS merged commit 95cf4d8 into OAI:OpenAPI.next Jun 9, 2017
@krishahn krishahn deleted the edit-3.0-format branch June 9, 2017 23:11
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.

8 participants