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 support for multi-format schema #814

Merged
merged 9 commits into from
Jul 28, 2023

Conversation

smoya
Copy link
Member

@smoya smoya commented Jul 24, 2023

Description
This PR adds the required changes for supporting Multi-format schemas.
The API changes are in thisparser-api PR asyncapi/parser-api#86 waiting to be reviewed.

Replaces #785

Related issue(s)
asyncapi/spec#622

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.

I won't to request changes to not block you, but I have some comments. Very well! :)

src/models/v2/message-trait.ts Outdated Show resolved Hide resolved
src/models/v2/schema.ts Outdated Show resolved Hide resolved
src/models/v2/schema.ts Show resolved Hide resolved
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Small comments 👍

@@ -26,20 +26,6 @@ describe('Message model', function() {
});
});

describe('.schemaFormat()', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the tests when we still have the functions? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The reality is that message should not have the schemaFormat method but only the MessageTrait. See https://github.com/asyncapi/parser-api/pull/86/files#diff-ac35ac6e0b0ad3629802a0e04076aa18e94da614f5392e1164dcaacdfe3ee198L157

Copy link
Member Author

@smoya smoya Jul 26, 2023

Choose a reason for hiding this comment

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

I think i will need to change the way inheritance is being made atm... Just adding a bit of composition with a intermediate model MessageBase that both Message + MessageTrait extend from.

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking again and again, makes sense to keep schemaFormat + add hasSchemaFormat to messageObject as well so we reduce the BC impact. I applied the changes + changes in parser-api. See asyncapi/parser-api#86 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

So this test method is back now

@@ -26,20 +20,6 @@ describe('Message model', function() {
});
});

describe('.schemaFormat()', function() {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the tests when we still have the functions? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

The reality is that message should not have the schemaFormat method but only the MessageTrait. See https://github.com/asyncapi/parser-api/pull/86/files#diff-ac35ac6e0b0ad3629802a0e04076aa18e94da614f5392e1164dcaacdfe3ee198L157

src/models/schema.ts Show resolved Hide resolved
@smoya
Copy link
Member Author

smoya commented Jul 26, 2023

@magicmatatjahu @jonaslagoni this is now ready for review again 🙏

@sonarcloud
Copy link

sonarcloud bot commented Jul 26, 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

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

LGTM, @magicmatatjahu do you want to have a look again?

@jonaslagoni
Copy link
Member

Merging, if you have anything @magicmatatjahu it can be addressed afterwards 🙂

@jonaslagoni jonaslagoni merged commit 9b79e90 into asyncapi:next-major-spec Jul 28, 2023
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.0-next-major-spec.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@smoya smoya deleted the feat/multiFormatSchema branch July 31, 2023 16:49
@asyncapi-bot
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants