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

refactor: add Avro Schema Parser #576

Merged
merged 4 commits into from
Aug 26, 2022

Conversation

smoya
Copy link
Member

@smoya smoya commented Aug 11, 2022

Description

This PR does the following:

Note
There are some TODOs I left in purpose to have a discussion.

Related issue(s)
#480


async function parse(input: ParseSchemaInput<unknown, unknown>): Promise<AsyncAPISchema> {
const asyncAPISchema = await avroToJsonSchema(input.data as AvroSchema);
const message = (input.meta as any).message
Copy link
Member Author

@smoya smoya Aug 11, 2022

Choose a reason for hiding this comment

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

The big change VS what we had previously is that the new interface passes directly the Schema in input.data rather than the message object.
I decided to add the message object in the input.meta and only in case it is there, do the modifications (if they are being done here finally).

cc @magicmatatjahu

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I will remember about that :) Avro parser there is a bit problematic 😅

@derberg
Copy link
Member

derberg commented Aug 16, 2022

I'm not involved in rewriting to TypeScript, so this might be silly question, but why are you porting avro schema parser here inside the parser?

@smoya
Copy link
Member Author

smoya commented Aug 16, 2022

I'm not involved in rewriting to TypeScript, so this might be silly question, but why are you porting avro schema parser here inside the parser?

Even though there are unit tests, the idea is to have everything here, to do proper testing of the parser with those schema parsers, and once we confirm everything works as expected, to move the code to each schema parser repo.

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 👍 Pinging @dalelane and @M3lkior as they're both code owners of the Avro schema parser repo.

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.

Great! I left some comments :) Please refer to them and we can merge it :)

src/schema-parser/avro-schema-parser.ts Outdated Show resolved Hide resolved
src/schema-parser/avro-schema-parser.ts Show resolved Hide resolved
src/schema-parser/avro-schema-parser.ts Show resolved Hide resolved
src/schema-parser/avro-schema-parser.ts Outdated Show resolved Hide resolved
src/schema-parser/avro-schema-parser.ts Outdated Show resolved Hide resolved
@smoya smoya requested review from magicmatatjahu and removed request for derberg, jonaslagoni and asyncapi-bot-eve August 25, 2022 09:04
@smoya smoya requested a review from magicmatatjahu August 25, 2022 15:46
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! 🚀 Should we wait for review of parser's maintainers?

@smoya
Copy link
Member Author

smoya commented Aug 26, 2022

We can merge now as we have two approvals from code owners.

@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

@smoya
Copy link
Member Author

smoya commented Aug 26, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 8c976e8 into asyncapi:next-major Aug 26, 2022
@smoya smoya deleted the feat/avroSchemaParser branch August 26, 2022 07:50
magicmatatjahu pushed a commit to magicmatatjahu/parser-js that referenced this pull request Oct 3, 2022
derberg pushed a commit that referenced this pull request Oct 4, 2022
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.

6 participants