Skip to content

Conversation

@char0n
Copy link
Member

@char0n char0n commented Aug 10, 2021

Refs #604

@sonarqubecloud
Copy link

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

@magicmatatjahu magicmatatjahu added the ✏️ Editorial PR is non-normative or does not influence implementation label Aug 10, 2021
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM @derberg @fmvilas do you want to add something?

@derberg
Copy link
Member

derberg commented Aug 10, 2021

it touches the specification, bug fix imho and therefore fix: that will have to trigger 2.1.1 release. We have the same issue in json schema -> https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/2.1.0.json#L934 instead of https://github.com/asyncapi/spec-json-schemas/blob/master/schemas/2.1.0.json#L699

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Aug 10, 2021

@derberg I think that it is editorial change, because we "missed" changing the description of the examples in the Message Trait Object, so docs: is enough for me - it other words, Message Trait Object inheritances the shape from Message Object, so we don't change its shape, but only fix it.

I also agree that json-schemas should be updated.

@derberg
Copy link
Member

derberg commented Aug 10, 2021

but only fix it

exactly, we fix it 😄 and fix is so significant that even json schema needs an update. This would be our first patch release so better to wait for @fmvilas opinion too. For now, it is 2.1.1 for me, pure bug fix in the specification, not simple editorial change.

@char0n is it ok for you to provide a fix in a JSON Schema too? Let us discuss there on PR but I guess to avoid "forgetting" things in the future, examples should be reused from definitions

@magicmatatjahu
Copy link
Member

but only fix it

I shouldn't write it 😆 For me it's editorial change.

@char0n
Copy link
Member Author

char0n commented Aug 10, 2021

@derberg yep I'll amend the metaschema as well

@char0n
Copy link
Member Author

char0n commented Aug 11, 2021

@derberg schema fixed in asyncapi/spec-json-schemas#85

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.

LGTM 👍

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM

@derberg
Copy link
Member

derberg commented Sep 20, 2021

@fmvilas you need to share your opinion on PR title as for me it is fix: and for @magicmatatjahu it is docs:

if we agree with fix: we should rather push it to release branch and release in a week together with 2.2 release

@fmvilas
Copy link
Member

fmvilas commented Sep 20, 2021

It's indeed a fix. The previous meaning was different than the current meaning.

@derberg
Copy link
Member

derberg commented Sep 20, 2021

@char0n can you please change base branch to release branch for this one, the one that points to September release?

@char0n char0n changed the base branch from master to 2021-09-release September 20, 2021 16:04
@char0n
Copy link
Member Author

char0n commented Sep 20, 2021

I've changed the base but I'd have to redo the PR so that unrelated commits are not visible here. I'll do it first thing tommorow morning.

@derberg
Copy link
Member

derberg commented Sep 20, 2021

rebasing with release branch should do the trick, no?

@char0n char0n force-pushed the char0n/message-examples-unification branch from 89b63d7 to 87f0b8f Compare September 20, 2021 17:33
@sonarqubecloud
Copy link

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
Copy link
Member Author

char0n commented Sep 20, 2021

PR force-pushed and rebased on top of 2021-09-release

@derberg derberg changed the title docs(spec): unify Message and MessageTrait examples description fix: unify Message and MessageTrait examples description Sep 21, 2021
@derberg derberg merged commit 6887699 into asyncapi:2021-09-release Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✏️ Editorial PR is non-normative or does not influence implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants