Add Schema examples annotation #580

gcheadle-vmware · 2022-01-12T00:17:57Z

include annotation lines in mismatched type error
collect mismatched type error all provided examples and return together

cari-lynn

Great job covering all the possible test cases. I'm really happy with the code added here, and think the description struct is a good way to group these annotations.

One piece for discussion:

There is two different syntax for adding an example WITHOUT a description.

@schema/examples (‘value’,)

@schema/examples (“”, “value”)
Since if the description is “” it will be ignored.

I do like the readability of the second one, I can't imagine a use case where the description needs to be "". But I do wonder if it is a good idea to remove the string just because it is zero value.. 🤔

pkg/schema/annotations.go

gcheadle-vmware · 2022-01-13T19:11:57Z

two different syntax for adding an example WITHOUT a description.

@schema/examples (‘value’,)

@schema/examples (“”, “value”)
Since if the description is “” it will be ignored.

@cari-lynn
I'm glad you pointed this out, I thought that it was not right to add an empty string as the value to the x-example-description key, so I treated the empty string as the default case. I could see a case where a GUI is displaying the example and it's description from an OpenAPI schema, and an author might need to always include the x-example-description key since the GUI checks for it. The author in that case may want the ability to include an empty string?

However the x-example-description key is a custom extension to the OpenAPI spec so this GUI would have to be built specifically for the schema that ytt generates...

return multiple type mismatches together if from the same examples annotation

pivotaljohn · 2022-01-18T18:42:03Z

@cari-lynn I'm glad you pointed this out, I thought that it was not right to add an empty string as the value to the x-example-description key, so I treated the empty string as the default case. I could see a case where a GUI is displaying the example and it's description from an OpenAPI schema, and an author might need to always include the x-example-description key since the GUI checks for it. The author in that case may want the ability to include an empty string?

Oh, as having consumed one-to-many APIs where keys disappear when no value is specified, I really appreciate this thinking. Reminds me of the Go Proverb, "Make the zero value useful".

However the x-example-description key is a custom extension to the OpenAPI spec so this GUI would have to be built specifically for the schema that ytt generates...

And it's the nature of maps/dictionaries/key-value pairs that such entries can easily/safely be ignored if not needed. 👍🏻

pkg/cmd/template/schema_author_test.go

pivotaljohn · 2022-01-18T22:43:01Z

pkg/cmd/template/schema_author_test.go

+
+    = found: non tuple in @schema/examples (by schema.yml:3)
+    = expected: tuple with optional string description and required example value
+    = hint: use a trailing comma to construct tuple with a single value. e.g. ('example value',)


I believe this would be a non-empty description with a None value...?

I think you mean to hint: (, "example value")

(I looked into this a bit so im commenting what I found) I thought ('example value',) was odd too. But I believe the trailing comma is the correct syntax. That makes it a single-element tuple. I believe the leading comma syntax isn't supported.
https://wiki.python.org/moin/TupleSyntax#:~:text=Multiple%20Element%20Tuples&text=Multiple%2Delement%20tuples%20may%20be,be%20enclosed%20in%20parentheses%2C%20e.g.

See comment below about disallowing this for now.

pkg/schema/annotations.go

pivotaljohn · 2022-01-18T23:04:16Z

pkg/schema/type.go

 	GetTitle() string
+	GetExample() (string, interface{})
+	SetDocumentation(documentation)


This signature will make it not possible for code outside of schema to be able to set documentation...

... have you already considered providing setters for Title and Description and then maybe something like AddExample(..)? or SetExamples(...)?

Related:

Today, we can only use the first example in OpenAPI v3.0 output

But when we turn around and support OpenAPI v3.1 (and that will happen), we'll want to be able to store multiple examples.

Also, we'd love to be able to support multiple examples in ytt Schema, today; no need to limit that part of these system.

Consider what happens when we push the limitation right up to the seam where it's introduced (i.e. when building the OpenAPI output).

cari-lynn · 2022-01-19T02:23:01Z

I thought that it was not right to add an empty string as the value to the x-example-description key, so I treated the empty string as the default case

I can see adding the x-example-description key adding noise to an already dense schema and not a lot of value.

And it's the nature of maps/dictionaries/key-value pairs that such entries can easily/safely be ignored if not needed. 👍🏻

Forgive me for being thick-skulled, do you mean that the empty string in the case of ("", "value") can be ignored if not needed or do you mean that OpenAPI schema consumers can ignore x-example-description if not needed?

For users that want to create a tuple without a description we could encourage users to use the ('example value',) syntax, while having the ('', 'example value') syntax behave as predicted and add x-example-description: "".

cari-lynn · 2022-01-19T02:25:18Z

One more thing I was experimenting with is using a yamlFragment in the annotation to clean things up:

#@schema/examples func()

But I believe there is no way to specify a tuple in yaml.. which makes abstracting this to a function not possible using yamlFragements.

pivotaljohn · 2022-01-19T17:08:49Z

GC: I thought that it was not right to add an empty string as the value to the x-example-description key, so I treated the empty string as the default case

CLD: I can see adding the x-example-description key adding noise to an already dense schema and not a lot of value.

I agree that if its not typically used, it's more noise than signal. But by making it optional (i.e. sometimes present, sometimes not), makes that "API" a bit more complicated to use (noted, below).

JSR: And it's the nature of maps/dictionaries/key-value pairs that such entries can easily/safely be ignored if not needed. 👍🏻

CLD: Forgive me for being thick-skulled, do you mean that the empty string in the case of ("", "value") can be ignored if not needed or do you mean that OpenAPI schema consumers can ignore x-example-description if not needed?

The latter: that it's (usually) easy to ignore an entry in a map. I'm drawing an analogy with JSON APIs: that it makes for an easier interface to program to when every time there's an example: property, it always has an x-example-description... even if an empty string. It's analogous to a field that's zero vs. not present/defined.

For users that want to create a tuple without a description we could encourage users to use the ('example value',) syntax, while having the ('', 'example value') syntax behave as predicted and add x-example-description: "".

Wouldn't that mean that sometimes the first element in the tuple is the example value and other-times it would be the second element in the tuple?

gcheadle-vmware · 2022-01-20T01:03:48Z

After syncing with John, I decided to go with a simpler design for the annotation, where the description and example are always required. If the user does not want to include a description, they can just provide an empty string and the x-example-description key will just have an empty string.

cari-lynn · 2022-01-20T20:08:44Z

Commenting here to wrap up the threads I started.

[pivotalJohn] it always has an x-example-description... even if an empty string

Sounds good. I agree with the decisions above 👍 If we hear from users that this is noisy, in the future we can add a flag or syntax to omit this.

[me] But I believe there is no way to specify a tuple in yaml.. which makes abstracting this to a function not possible using yamlFragements.

I am satisfied with being able to use a syntax to declare the example in a yamlFragment, and not necessarily being able to declare the entire tuple in a yamlFragment. Eg:
#@schema/example ("host", hostExample())

Thank you!!

- simplify example API to always require string description and valid example - accept null as a valid example value for nullable data values - reword error messages

gcheadle-vmware · 2022-01-24T22:08:13Z

@pivotaljohn, @cari-lynn - changes made to type interface to reflect the conversation earlier today.

pivotaljohn

Looks great, @gcheadle-vmware.

Note the minor interaction with #582 — there's a hint in here that (should) go away when it merges.

Will leave it to you to decide which order you want these merged to minimize the effort on your part. Ping me if you want a buddy in that.

pivotaljohn · 2022-01-24T23:08:24Z

pkg/cmd/template/schema_author_test.go

+
+    = found: integer (by schema.yml:3)
+    = expected: boolean (by schema.yml:4)
+    = hint: is the default value set using @schema/default?


Just note: the hint is misplaced and is fixed in #582. This is a new instance of the hint and will need to be removed after that PR is merged.

vmwclabot added the cla-not-required label Jan 12, 2022

gcheadle-vmware marked this pull request as ready for review January 12, 2022 19:09

gcheadle-vmware requested review from pivotaljohn and cari-lynn January 12, 2022 19:09

gcheadle-vmware mentioned this pull request Jan 13, 2022

Include default annotation position when default value is wrong type #582

Merged

cari-lynn approved these changes Jan 13, 2022

View reviewed changes

pkg/schema/annotations.go Outdated Show resolved Hide resolved

gcheadle-vmware added 9 commits January 13, 2022 15:27

Add @schema/examples annotation to openAPI schema output

f408a55

refactor: use struct to set description and examples in one

b669cd9

typecheck nest yaml in schema/examples

0c8316e

add logic around collecting multiple examples

15ea926

add @schema/examples syntax error tests

e37810e

add ann positions to mismatchedTypeError

3101029

return multiple type mismatches together if from the same examples annotation

refactor: extract function to type and check examples

255eabb

add singleton examples and error when not tuple

5b72996

refactor @schema/title into struct with other documentation

a321bea

gcheadle-vmware force-pushed the schema-examples-annotation branch from b9549aa to a321bea Compare January 14, 2022 00:44

reorder keys in open API schema

490144b

pivotaljohn suggested changes Jan 18, 2022

View reviewed changes

Respond to feedback

5e32602

- simplify example API to always require string description and valid example - accept null as a valid example value for nullable data values - reword error messages

gcheadle-vmware force-pushed the schema-examples-annotation branch from 79702f4 to 5e32602 Compare January 20, 2022 20:09

gcheadle-vmware added 2 commits January 20, 2022 15:28

retain full example list provided via @schema/examples

f5c2382

Refactor type interface to set metadata individually

2f6d5e0

pivotaljohn approved these changes Jan 24, 2022

View reviewed changes

gcheadle-vmware merged commit a5dfa11 into develop Jan 24, 2022

gcheadle-vmware deleted the schema-examples-annotation branch January 24, 2022 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Schema examples annotation #580

Add Schema examples annotation #580

gcheadle-vmware commented Jan 12, 2022 •

edited

cari-lynn left a comment

gcheadle-vmware commented Jan 13, 2022 •

edited

pivotaljohn commented Jan 18, 2022

pivotaljohn Jan 18, 2022

cari-lynn Jan 19, 2022

gcheadle-vmware Jan 20, 2022

pivotaljohn Jan 18, 2022

cari-lynn commented Jan 19, 2022

cari-lynn commented Jan 19, 2022

pivotaljohn commented Jan 19, 2022

gcheadle-vmware commented Jan 20, 2022

cari-lynn commented Jan 20, 2022

gcheadle-vmware commented Jan 24, 2022

pivotaljohn left a comment

pivotaljohn Jan 24, 2022

Add Schema examples annotation #580

Add Schema examples annotation #580

Conversation

gcheadle-vmware commented Jan 12, 2022 • edited

cari-lynn left a comment

Choose a reason for hiding this comment

gcheadle-vmware commented Jan 13, 2022 • edited

pivotaljohn commented Jan 18, 2022

pivotaljohn Jan 18, 2022

Choose a reason for hiding this comment

cari-lynn Jan 19, 2022

Choose a reason for hiding this comment

gcheadle-vmware Jan 20, 2022

Choose a reason for hiding this comment

pivotaljohn Jan 18, 2022

Choose a reason for hiding this comment

cari-lynn commented Jan 19, 2022

cari-lynn commented Jan 19, 2022

pivotaljohn commented Jan 19, 2022

gcheadle-vmware commented Jan 20, 2022

cari-lynn commented Jan 20, 2022

gcheadle-vmware commented Jan 24, 2022

pivotaljohn left a comment

Choose a reason for hiding this comment

pivotaljohn Jan 24, 2022

Choose a reason for hiding this comment

gcheadle-vmware commented Jan 12, 2022 •

edited

gcheadle-vmware commented Jan 13, 2022 •

edited