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: integrate new parser-api #833

Closed

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Sep 15, 2022

Description

Integrate new ParserJS API:

  • add new template config field apiVersion.
  • update relevant source code to handle new and old API
  • add some utils
  • update tests

Related issue(s)
Resolves #825
Part of asyncapi/parser-js#481
Blocked by stoplightio/spectral#2284

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Sep 15, 2022
@magicmatatjahu magicmatatjahu marked this pull request as ready for review November 7, 2022 14:19
@magicmatatjahu magicmatatjahu changed the title feat: integrate new parser API feat: integrate new parser-api Nov 7, 2022
fmvilas
fmvilas previously approved these changes Nov 14, 2022
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 👍

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 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
0.0% 0.0% Duplication

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.

I think I do not understand something 😓
how does the new parser version relate to custom parsers? I'm confused 😄
I left this comment in avro schema parser in the past asyncapi/avro-schema-parser#168 (review)

also, since asyncapi/.github#204 the bump.yml workflow is not going to be replicated in repositories that do not have get-global-node-release-workflows. This means that you can now easily update it in parser repository, in the master branch to make sure regular releases will not revert the version update that you did here.

@sonarcloud
Copy link

sonarcloud bot commented Feb 1, 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
0.0% 0.0% Duplication

@@ -19,7 +19,7 @@ describe('Integration testing generateFromFile() to make sure the result of the
return path.resolve(mainTestResultPath, crypto.randomBytes(4).toString('hex'));
};

jest.setTimeout(30000);
jest.setTimeout(60000);
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to increate timeout to 60s, because on windows integration tests always failed - first test create snapshot and the next one check that snapshot, so at the end all tests failed.

@magicmatatjahu
Copy link
Member Author

@derberg @jonaslagoni I think that we can merge that PR :) I wanna merge it after v2.6.0 release, because then we will have time to fix any potenial bugs before next spec release.

derberg
derberg previously approved these changes Feb 6, 2023
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.

💪🏼

@jonaslagoni
Copy link
Sponsor Member

@magicmatatjahu got some conflicts.

@smoya
Copy link
Member

smoya commented Mar 9, 2023

@magicmatatjahu do you think you will still be able to move forward with this PR? 🙏

@sonarcloud
Copy link

sonarcloud bot commented Mar 20, 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
0.0% 0.0% Duplication

@jonaslagoni
Copy link
Sponsor Member

FInal round of review before merging, I solved the conflicts 🙂

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.

docs wise I was thinking if we need something in https://github.com/asyncapi/generator/blob/master/docs/parser.md but maybe it doesn't make sense as we do not want to have it here for long, I guess

lgtm

@smoya
Copy link
Member

smoya commented Mar 29, 2023

@magicmatatjahu can you please update the branch with latest master changes?

@smoya
Copy link
Member

smoya commented May 2, 2023

As discussed offline via slack DM, @magicmatatjahu is happy with me creating a new PR with all the new changes needed since it can't handle it atm.

This then can be closed in favor of #960

cc @jonaslagoni @fmvilas @derberg

@derberg
Copy link
Member

derberg commented May 11, 2023

closing in favour of #960

@derberg derberg closed this May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate new ParserAPI
5 participants