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

Jump to bottom

docs: clarify the resolution rules of Channel Item $ref field #779

Merged
merged 2 commits into from
Sep 7, 2022
Merged

docs: clarify the resolution rules of Channel Item $ref field #779

merged 2 commits into from
Sep 7, 2022

Conversation

char0n
Copy link
Collaborator

@char0n char0n commented May 4, 2022

Refs #612

@char0n char0n force-pushed the char0n/channel-item--clarification branch from 53d7f67 to d165856 Compare May 4, 2022 08:25
@char0n char0n changed the title docs: clarify the resolution rules of Channel Item $ref field docs: clarify the resolution rules of Channel Item $ref field May 4, 2022
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 4, 2022

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

@char0n char0n mentioned this pull request May 4, 2022
Closed
@fmvilas
Copy link
Member

fmvilas commented May 5, 2022

I think this PR can be removed in favor of #777?

@char0n
Copy link
Collaborator Author

char0n commented May 5, 2022

@fmvilas this change is for 2.x branch, the #777 is for 3.x branch. Assuming we don't know exactly how soon the 2.x branch of the spec goes away and it will clarify for 2.x consumers what the resolutions rules are.

@char0n char0n mentioned this pull request May 6, 2022
Open
@derberg
Copy link
Member

derberg commented May 10, 2022

is it really needed? taking into account that the field is anyway deprecated, which means "do not use it"

@char0n
Copy link
Collaborator Author

char0n commented May 10, 2022

@derberg it's up to you to decide. I can only provide reasons why it would be maybe appropriate:

The only reason I created the PR is that I work on tooling implementation and I needed to implement resolution mechanism for this. I made an assumption that resolution here adheres to JSON Reference spec. This was not clear from the specification for me. Even though we say "do not use it" the mechanism is there and it should be crystal clear how it works, even though it's deprecated.

This doesn't change the specification in any way, it just clarifies something that is currently up to implement-or interpretation.

@fmvilas
Copy link
Member

fmvilas commented Jun 9, 2022

@fmvilas this change is for 2.x branch, the #777 is for 3.x branch. Assuming we don't know exactly how soon the 2.x branch of the spec goes away and it will clarify for 2.x consumers what the resolutions rules are.

Sorry for the confusion, @char0n. You're right.

fmvilas
fmvilas approved these changes Jun 9, 2022
View reviewed changes
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.

Looks good to me 👍

@char0n char0n mentioned this pull request Sep 6, 2022
Closed
@char0n
Copy link
Collaborator Author

char0n commented Sep 6, 2022

@derberg @dalelane can I ask you to look into this PR? Thanks!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 7, 2022

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
derberg approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

sorry for keeping you waiting 😞

this one doesn't have to wait for 2.5 as this is an editorial change that points to master

/rtm

@derberg
Copy link
Member

derberg commented Sep 7, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 9a293c7 into asyncapi:master Sep 7, 2022
@char0n char0n mentioned this pull request Sep 13, 2022
Closed
61 tasks
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-next-major-spec.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@fmvilas
Copy link
Member

fmvilas commented Sep 22, 2022

Forget about the last comment saying it was released in version 2.5.0-next-major-spec.1. I made a mistake and it created this version but it should actually be 3.0.0-next-major-spec.1. There's a notice in the release. Apologies for the noise.

@derberg derberg mentioned this pull request Sep 28, 2022
Merged
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.5.0-next-spec.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@derberg
Copy link
Member

derberg commented Jan 31, 2023

Recent comments about the release from the bot were added by mistake. More details in #899

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derberg derberg derberg approved these changes

@dalelane dalelane dalelane approved these changes

@fmvilas fmvilas fmvilas approved these changes

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

Assignees
No one assigned
Labels
ready-to-merge released on @next-spec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@char0n @fmvilas @derberg @asyncapi-bot @dalelane