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

feat: add info that protocol binding objects are extendable #893

Closed
wants to merge 3 commits into from
Closed

feat: add info that protocol binding objects are extendable #893

wants to merge 3 commits into from

Conversation

derberg
Copy link
Member

@derberg derberg commented Jan 17, 2023

Resolves #869

I wasn't sure what is the best way to specify it on binding level in the spec. Open to different ideas 🙏🏼

@fmvilas @char0n @dalelane @smoya

I will also open separate PR to bindings repo to make it clear there for every single binding

dalelane
dalelane previously approved these changes Jan 17, 2023
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

This seems like a good approach to me

@char0n
Copy link
Collaborator

char0n commented Feb 5, 2023

Hi @derberg,

The usual position for the sentence "This object MAY be extended with [Specification Extensions](#specificationExtensions)" is right after the list of fixed fields. In this PR you've changed this rule and IMHO this might bring future confusion. If I for example, as a tooling author, don't find this sentence right after the list of object fixed fields, I'll automatically assume object doesn't support the spec extensions. Consistency is the king ;]

image


Protocol-specific binding objects MAY be extended with Specification Extensions.

Regarding the above sentence - it might conflict with what the protocol specific binding objects actually demand (some demands no spec extensions). What would be the source of truth for this conflict resolution then? I'd assume the bindings overrides what AsyncAPI spec says.

IMHO the best solution would be to retain consistency and for every protocol specific binding object we should either add the already established This object MAY be extended with [Specification Extensions](#specificationExtensions) or not (if not suitable).

This would make it compatible with AsyncAPI spec and if protocol specific binding supports the spec extension, people will know where to find this information and will know what it means when this information is missing.

@derberg
Copy link
Member Author

derberg commented Feb 6, 2023

@char0n do you mean we do not need this PR and Protocol-specific binding objects MAY be extended with [Specification Extensions](#specificationExtensions). is not needed?

the goal of this sentence is to enforce a standard on all the bindings, that they need to be extensible, that it is not individual binding maintainer to disable extensions. If we do not have this here in the spec, how can we ensure it in bindings? this rule should be written in spec IMHO, not in some contribution guide in the bindings repo


and yeah, I totally agree that consistency matters, I just did not have a better idea. I do not know why but sometimes having below under the table with bindings list seemed strange to me:

This object MAY be extended with [Specification Extensions](#specificationExtensions).
Protocol-specific binding objects MAY be extended with [Specification Extensions](#specificationExtensions).

but now when I think of it, it looks good 😄

@char0n
Copy link
Collaborator

char0n commented Feb 8, 2023

@char0n do you mean we do not need this PR and Protocol-specific binding objects MAY be extended with Specification Extensions. is not needed?

Yep, that's I'm trying to say.

the goal of this sentence is to enforce a standard on all the bindings, that they need to be extensible, that it is not individual binding maintainer to disable extensions. If we do not have this here in the spec, how can we ensure it in bindings? this rule should be written in spec IMHO, not in some contribution guide in the bindings repo

I understand your motivation, but IMHO this sentence tries to convey how new bindings should be constructed, which should be part of PR review process to make sure bindings are consistent and always include the This object MAY be extended with [Specification Extensions](#specificationExtensions) sentence.

I see also an ambiguity in Protocol-specific binding objects MAY be extended with [Specification Extensions](#specificationExtensions). - does this mean that protocol bindings which are currently reserved supports the extension or not?

You've already issued asyncapi/bindings#179, that handles extension on individual protocol binding level. Every binding object that can be extended with extension explicitly states that now. This is consistent with AsyncAPI spec itself and it should be enough for tooling authors.


Anyway I don't have strong opinion about this. If you leave the sentence there it's just fine. I was just trying to possibly bring a little bit more consistency and clarity, and eliminate ambiguity.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@derberg
Copy link
Member Author

derberg commented Feb 8, 2023

Ok so what I did for now is moving info about binding extensibility back to its original location for consistency. The only change you can see is a new sentence about extensibility of specific binding object.

@dalelane @smoya @fmvilas
can you folks look on the message from @char0n ?

I'm in doubt. Cause yes, adding the info I suggest is weird. But on the other hand, we cannot just count on the PR review process and that having extensions enabled everywhere is just a matter of consistency. I think we need to have it written somewhere, and still think bindings repo contribution guide is not a place for that

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

I think it's totally fine to put it here.

Copy link
Member Author

derberg commented Mar 15, 2023

@smoya can you also have a look please?

@derberg derberg closed this by deleting the head repository Mar 30, 2023
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