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.

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

docs: channel object / $ref note. #826

Merged
merged 8 commits into from
Oct 18, 2023
Merged

docs: channel object / $ref note. #826

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8733388
Update asyncapi.md
ponelat Aug 22, 2022
3dfb71d
Update asyncapi.md
ponelat Aug 22, 2022
c8f2cd4
Update asyncapi.md
ponelat Aug 23, 2022
4d113c2
Update asyncapi.md
ponelat Sep 5, 2022
235b242
Update spec/asyncapi.md
derberg Jul 20, 2023
a42281e
Merge branch 'master' into patch-1
derberg Jul 20, 2023
2425fc5
Merge branch 'master' into patch-1
derberg Sep 28, 2023
50dbafe
fix markdown lint
derberg Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/asyncapi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ Describes the operations available on a single channel.

Field Name | Type | Description
---|:---:|---
<a name="channelItemObjectRef"></a>$ref | `string` | Allows for an external definition of this channel item. The referenced structure MUST be in the format of a [Channel Item Object](#channelItemObject). If there are conflicts between the referenced definition and this Channel Item's definition, the behavior is *undefined*. <br/><br/>**Deprecated:** Usage of the `$ref` property has been deprecated.
<a name="channelItemObjectRef"></a>$ref | `string` | Allows for an external definition of this channel item. The referenced structure MUST be in the format of a [Channel Item Object](#channelItemObject). If there are conflicts between the referenced definition and this Channel Item's definition, the behavior is *undefined*. <br/><br/>**Technical Note:** Going forward, `$ref` will no longer form part of these _fixed fields_, but will continue to function in the same role, as part of [Reference Object](#referenceObject).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @ponelat,

Thanks for the proposal, here are couple of notes I have to this change:

Removal of deprecation message

If we remove the deprecation message, it's going to look like a cosmetic change, but it's not. We're actually removing a fixed field from the Channel Item Object, which will change the way, how the eventual transcluded referenced value shape will look like (merging). I think we can make the deprecation message more precise by doing following change to clarify the deprecation scope:

**Deprecated:** Usage of the `$ref` property has been deprecated when accompanied with one or more fixed fields from Channel Item Object`.

We did the same change in OpenAPI specification some time ago to Path Item Object - https://github.com/OAI/OpenAPI-Specification/pull/2656/files

Self containment

The wording of the change is IMHO not self-contained. The wording inside one version of the spec should probably not refer to the future version of the spec - OAI/OpenAPI-Specification#2656 (comment)

Different behavior

but will continue to function in the same role, as part of [Reference Object](#referenceObject) - this might not be true. Reference Object does't allow additional fields, which means, no merging with referenced value is performed. ChannelItem.$ref does allow additional fields, which gets merged with the referenced value. If there is a conflict, the behavior is undefined, but most implementations do merge using right shallow merge algorithm.

Copy link
Contributor Author

@ponelat ponelat Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @char0n ,

Removal of deprecation message
Understood, although without highlighting how it can be used outside of fixed fields, then it reads as "Don't use $ref here".

Perhaps a tweak on your suggestion:

Deprecated: Using $ref with sibling fixed fields has been deprecated. Usage without any siblings is still encouraged!
The motivation behind changing the wording: highlighting that a specific usage of $ref is deprecated.

Self containment and Different behavior. Understood and agreed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The tweak sounds great, and is aligned with #699

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the change.

Copy link
Member

Choose a reason for hiding this comment

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

I have to disagree with you. IMHO, the sentence Using $ref with sibling fixed fields has been deprecated. Usage without any siblings is still encouraged! is very difficult to understand. In particular, the word siblings and referring to fixed fields sounds confusing to me.

WDYT about this version?

Usage of the $ref property has been deprecated when accompanied by any of the other properties of a Channel Item Object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @smoya,

Usage of the $ref property has been deprecated when accompanied by any of the other properties of a Channel Item Object.

When using https://grammarly.com/ to analyze the sentence, there are two suggestions to it:

  1. Passive voice - This sentence appears to be written in the passive voice. Consider writing in the active voice.
  2. Wordy sentence - Too many non-content words may indicate wordiness. Consider rewriting to avoid some of these words: of, the, has, been, when, by, any, a

IMHO @ponelat intention was to make explicit that if you use $ref without any other properties nothing will change in the future.

I propose another suggestion that passes grammarly and is less technical (possibly less confusing as well):

Using $ref property has been deprecated when accompanied by other properties.

This doesn't suffer from passive voice and wordy sentence issues and it aligned with your suggestion. properties is already used within the spec to refer to fixed or pattern fields. For me personally it also implicitly communicates that using $ref with no additional properties is not deprecated and will continue to functional equivalently, but IMHO it's worth considering if we want to communicate explicitly as @ponelat did in his suggestion as a second sentence.

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy with:

Using $ref property has been deprecated when accompanied by other properties.

Let's see what others say.

cc @alequetzalli @derberg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, but used field instead of property, to be consistent with the column name Field name and the section Fixed Fields.

<a name="channelItemObjectDescription"></a>description | `string` | An optional description of this channel item. [CommonMark syntax](https://spec.commonmark.org/) can be used for rich text representation.
<a name="channelItemObjectServers"></a>servers | [`string`] | The servers on which this channel is available, specified as an optional unordered list of names (string keys) of [Server Objects](#serverObject) defined in the [Servers Object](#serversObject) (a map). If `servers` is absent or empty then this channel must be available on all servers defined in the [Servers Object](#serversObject).
<a name="channelItemObjectSubscribe"></a>subscribe | [Operation Object](#operationObject) | A definition of the SUBSCRIBE operation, which defines the messages produced by the application and sent to the channel.
Expand Down