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

3.0.4: port the undisputed parts of #2140 #4062

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

ralfhandl
Copy link
Contributor

@ralfhandl ralfhandl commented Aug 30, 2024

This is an attempt to revive/port

It contains all undisputed changes:

  • rework confusing description in example
  • remove confusing reference to the Schema Object's treatment of default values

Part of

@@ -488,7 +488,7 @@ An object representing a Server Variable for server URL template substitution.
| Field Name | Type | Description |
| ---- | :----: | ---- |
| <a name="server-variable-enum"></a>enum | [`string`] | An enumeration of string values to be used if the substitution options are from a limited set. The array SHOULD NOT be empty. |
| <a name="server-variable-default"></a>default | `string` | **REQUIRED**. The default value to use for substitution, which SHALL be sent if an alternate value is _not_ supplied. Note this behavior is different than the [Schema Object's](#schema-object) treatment of default values, because in those cases parameter values are optional. If the [`enum`](#server-variable-enum) is defined, the value SHOULD exist in the enum's values. |
| <a name="server-variable-default"></a>default | `string` | **REQUIRED**. The default value to use for substitution, which SHALL be sent if an alternate value is _not_ supplied. If the [`enum`](#server-variable-enum) is defined, the value SHOULD exist in the enum's values. |
Copy link
Contributor

Choose a reason for hiding this comment

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

While the sentence that was delete here is a bit confusing, I think there is something lost if we don't call out the difference in the meaning of "default" here. Would something like this be okay?

Note this differs from a default value in the Schema Object's, which specify the value applied by the server when none is provided by the client.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The preceding sentence says

SHALL be sent

That is a MUST, not sending a value is not an allowed option.

The Schema Object's default talks about what

would be assumed [...] if one is not provided

Referencing something that does not apply is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are referencing something to explain that, while it looks very similar -- same property name default in a very similar context -- it is in fact different and does not apply. I think that will be helpful to users of the spec, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darrelmiller suggested to remove this sentence in #2140 (comment).

I agree because I find it confusing here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to change the sentence to something like:

Note that this behavior is different from the Schema Object's default keyword, which documents the default behavior rather than inserting the value into the data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this behavior is different from the Schema Object's default keyword, which documents the default behavior rather than inserting the value into the data.

That sounds good to me. However I don't really have a problem with Ralf's PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that keeping this sentence here is the option that was least favored by both @darrelmiller and the PR author @tedepstein, see his comment.

Copy link
Member

Choose a reason for hiding this comment

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

@ralfhandl I'm certainly not proposing keeping the current sentence, which I also find confusing. But people find default endlessly confusing. They are particularly likely to think that JSON Schema's default means that the value of default should be written into the instance, which is not how it works at all.

So the fact that we have a different default that does work that way is a very, very likely point of confusion. The text that was there before did not really help. However, I think the text that I proposed explains the real difference succinctly, and will keep people who read this default from thinking that JSON Schema's default works similarly. Which was always one of the endless recurring questions/complaints in the JSON Schema project, so it's a very practical and real concern, with years of experience answering this exact question to back it up.

I think my experience here is a bit deeper than was available during the previous conversation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@handrews Good point, have added your proposed sentence to the end of the table cell's text, and used "receiver's behavior" instead of "default behavior" to stress the difference.

miqui
miqui previously approved these changes Sep 1, 2024
@@ -488,7 +488,7 @@ An object representing a Server Variable for server URL template substitution.
| Field Name | Type | Description |
| ---- | :----: | ---- |
| <a name="server-variable-enum"></a>enum | [`string`] | An enumeration of string values to be used if the substitution options are from a limited set. The array SHOULD NOT be empty. |
| <a name="server-variable-default"></a>default | `string` | **REQUIRED**. The default value to use for substitution, which SHALL be sent if an alternate value is _not_ supplied. Note this behavior is different than the [Schema Object's](#schema-object) treatment of default values, because in those cases parameter values are optional. If the [`enum`](#server-variable-enum) is defined, the value SHOULD exist in the enum's values. |
| <a name="server-variable-default"></a>default | `string` | **REQUIRED**. The default value to use for substitution, which SHALL be sent if an alternate value is _not_ supplied. If the [`enum`](#server-variable-enum) is defined, the value SHOULD exist in the enum's values. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this behavior is different from the Schema Object's default keyword, which documents the default behavior rather than inserting the value into the data.

That sounds good to me. However I don't really have a problem with Ralf's PR as is.

@ralfhandl ralfhandl changed the title 3..0.4: port the undisputed parts of #2140 3.0.4: port the undisputed parts of #2140 Sep 2, 2024
@ralfhandl
Copy link
Contributor Author

@mikekistler does the added explanation meet your expectations?

@miqui miqui merged commit d1ba364 into OAI:v3.0.4-dev Sep 5, 2024
1 check passed
@ralfhandl ralfhandl deleted the 3.0.4-rework-pr-#2140 branch September 5, 2024 15:13
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