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: enable string input for generate #1039

Merged
merged 21 commits into from
Sep 28, 2023
Merged

feat: enable string input for generate #1039

merged 21 commits into from
Sep 28, 2023

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Sep 27, 2023

Description
This feature was driven by the following problems that needed to be fixed:

  • In feat: add Parser-API version 2 support #1017 I did not know that CLI used generateFromString instead of generate, and that was not changed at all.
  • generateFromString parses the document with the old parser, and that means it cannot understand v3 documents.

Changes:

  • generateFromString is being deprecated and generate should be used instead.
  • generate now handles string inputs and parses the document according to the template configuration apiVersion
  • validateTemplateConfig now accesses the server object according to the parser API

Related issue(s)
Fixes #1038

@jonaslagoni jonaslagoni marked this pull request as ready for review September 27, 2023 08:43
@jonaslagoni
Copy link
Member Author

I cant for the life of me disable or ignore the sonar cloud issues. If anyone knows how do let me know..

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.

in case of sonar, just remove these NOSONAR comments and modify https://github.com/asyncapi/generator/blob/master/.sonarcloud.properties

just exclude all tests like test/**/*

for cognitive complexity, put this before the function /* eslint-disable sonarjs/cognitive-complexity */


also please adjust title, it is a feat imho because of deprecation. So write what you change and not what you fix

lib/generator.js Outdated Show resolved Hide resolved
@jonaslagoni
Copy link
Member Author

for cognitive complexity, put this before the function /* eslint-disable sonarjs/cognitive-complexity */

This is not respected by sonar.

just exclude all tests like test/**/*

Added 👍

also please adjust title, it is a feat imho because of deprecation. So write what you change and not what you fix

Alright 👍

@jonaslagoni jonaslagoni changed the title fix: could not parse v3 documents even with apiVersion 2 feat: enable string input for generate Sep 27, 2023
@jonaslagoni jonaslagoni requested a review from derberg September 27, 2023 10:15
@derberg
Copy link
Member

derberg commented Sep 27, 2023

@jonaslagoni chatGPT wrote it is possible to add sonar.cpd.cognitivecomplexity.threshold=20 to .sonarcloud.properties

@jonaslagoni
Copy link
Member Author

@derberg yea but that will just increase global threshold, not really ignoring single function 😅

@derberg
Copy link
Member

derberg commented Sep 27, 2023

@jonaslagoni which is ok right? better just increase threshold, and then we should worry when we go beyond, and see what we can do, rather than just silent forever?

@jonaslagoni
Copy link
Member Author

@jonaslagoni which is ok right? better just increase threshold, and then we should worry when we go beyond, and see what we can do, rather than just silent forever?

I guess 😄 Updated it.

@derberg
Copy link
Member

derberg commented Sep 28, 2023

looks like chatGPT lied 😅 yeah, remove the fake setting (including // eslint-disable-next-line sonarjs/cognitive-complexity) and open a followup good first issue to solve complexity issue - and let us merge

@jonaslagoni
Copy link
Member Author

looks like chatGPT lied 😅 yeah, remove the fake setting (including // eslint-disable-next-line sonarjs/cognitive-complexity) and open a followup good first issue to solve complexity issue - and let us merge

Had to leave the eslint-disable-next-line as otherwise our linting tool would complain.

Did a quick test with the HTML template re-write just to make sure everything still worked as expected with v3 documents. And found a problem with tests and implementation.

@derberg should be all ready for the final review now ✌️

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.

Did you check with other templates, that are not yet migrated, like for example nodejs-template

I'm getting:

# running from local from fix_v3_support branch
$ ./cli.js https://bit.ly/asyncapi @asyncapi/nodejs-template -p server=production -o lolo --force-write
Something went wrong:
TypeError [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received undefined
    at Object.writeFileSync (node:fs:2146:5)
    at createAsyncapiFile (/Users/xxx/generator/node_modules/@asyncapi/generator-hooks/src/index.js:28:6)
    at /Users/xxx/generator/lib/generator.js:827:20
    at Array.map (<anonymous>)
    at Generator.launchHook (/Users/xxx/generator/lib/generator.js:824:28)
    at Generator.generate (/Users/xxx/generator/lib/generator.js:208:18)
    at async /Users/xxx/generator/cli.js:159:9

@derberg
Copy link
Member

derberg commented Sep 28, 2023

error comes from at createAsyncapiFile (/Users/xxx/generator/node_modules/@asyncapi/generator-hooks/src/index.js:28:6)

console log in the location

  console.log('dupa', asyncapi)
  fs.writeFileSync(asyncapiOutputLocation, asyncapi);

returns undefined

asyncapi variable comes from const asyncapi = generator.originalAsyncAPI;

and yeah, I checked, generator no longer provides originalAsyncAPI

@jonaslagoni
Copy link
Member Author

@derberg not sure why I removed it 🤔

@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 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jonaslagoni
Copy link
Member Author

Fixed it and added it to the tests

@jonaslagoni jonaslagoni requested a review from derberg September 28, 2023 10:23
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.

lgtm! 🚀

will you create the followup for cognitive stuff or want me to?

@jonaslagoni
Copy link
Member Author

will you create the followup for cognitive stuff or want me to?

If you got time please do 👍 Heading over to the templates right now to fix the remaining stuff so they are ready to be reviewed as well.

@jonaslagoni
Copy link
Member Author

Thanks @derberg 👍

@jonaslagoni
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit 553b5e2 into master Sep 28, 2023
16 checks passed
@asyncapi-bot asyncapi-bot deleted the fix_v3_support branch September 28, 2023 10:30
@derberg
Copy link
Member

derberg commented Sep 28, 2023

done #1040

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.13.0 🎉

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.

apiVersion 2 only works with AsyncAPI v2 documents
3 participants