Skip to content

feat: integrate modelina to generate models#173

Merged
asyncapi-bot merged 33 commits into
asyncapi:masterfrom
jonaslagoni:feature/modelina_integration
Jun 29, 2022
Merged

feat: integrate modelina to generate models#173
asyncapi-bot merged 33 commits into
asyncapi:masterfrom
jonaslagoni:feature/modelina_integration

Conversation

@jonaslagoni

@jonaslagoni jonaslagoni commented Jan 5, 2022

Copy link
Copy Markdown
Member

Description
This PR adds the basic command for generating models with Modelina to all support languages.

It follows:

## In memory generation
asyncapi generate models typescript ./asyncapi.json

## To files
asyncapi generate models typescript ./asyncapi.json --output='./output'
asyncapi generate models javascript ./asyncapi.json --output='./output'
asyncapi generate models java ./asyncapi.json --packageName='my.custom.package'  --output='./output'
asyncapi generate models csharp ./asyncapi.json --namespace='my.custom.namespace'  --output='./output'
asyncapi generate models golang ./asyncapi.json --packageName='my.custom.package'  --output='./output'
asyncapi generate models dart ./asyncapi.json --output='./output'

Related issue(s)
fixes #129
blocked by asyncapi/modelina#540
blocked by asyncapi/modelina#536

Comment thread src/commands/types.ts Outdated
@jonaslagoni jonaslagoni marked this pull request as ready for review January 11, 2022 12:04
@jonaslagoni

Copy link
Copy Markdown
Member Author

PR is ready to be reviewed 🙂

Comment thread src/commands/generate/types.ts Outdated
@derberg

derberg commented Jan 12, 2022

Copy link
Copy Markdown
Member

@magicmatatjahu @fmvilas @Souvikns hey folks, can you remind me as I can find it anywhere but my brain is telling me that we had a discussion about this principle that something should be a flag only if it can be optional and if something cannot be optional, and no default is possible then it should be part of the command. Referring to language flag

@Souvikns I remember that at some point in time we were discussing that commands should be stored in separate files so they are then reused in source and tests and not duplicated. Was it done and lost during refactoring or? can't remembers, sorry :(

@Souvikns

Copy link
Copy Markdown
Member

@derberg is #59 (comment) what you are looking for

@derberg

derberg commented Jan 12, 2022

Copy link
Copy Markdown
Member

@Souvikns not, not this one, this is about helping out if you do asyncapi generte and error ask if you meant asyncapi generate

@fmvilas

fmvilas commented Jan 12, 2022

Copy link
Copy Markdown
Contributor

Yeah I can't find where we discussed that either. I think --file should not be a flag but an argument instead. Something like asyncapi generate types <asyncapi-file>. Same applies to language, unless we want to provide a default value.

@magicmatatjahu

magicmatatjahu commented Jan 13, 2022

Copy link
Copy Markdown
Member

Yeah I would also be in favor of making the asyncapi file and language optional, so the call itself should look like this:

asyncapi generate types ./asyncapi.json typescript -o ./output

I also wonder if that long goPackageName, javaPackageName or csharpNamespace flags are needed. I know this is because of the different types of modules in the languages, but maybe packageName and namespace is enough? Another languages have namespace or module (packages) system so maybe we should go only with that two flags and reuse them with appropriate language 🤔

I also wonder if we should make as followup flag to load JS file to load config for given Langauge Generator? As you know user has possibility to change format of model name etc. and of course that config we cannot convert to the CLI flags/parameters. WDYT?

@derberg

derberg commented Jan 13, 2022

Copy link
Copy Markdown
Member

yeah, actually should not there be support for file/context/fallback commands anyway?

Comment thread src/commands/generate/types.ts Outdated
@jonaslagoni

jonaslagoni commented Jan 18, 2022

Copy link
Copy Markdown
Member Author

I also wonder if that long goPackageName, javaPackageName or csharpNamespace flags are needed. I know this is because of the different types of modules in the languages, but maybe packageName and namespace is enough? Another languages have namespace or module (packages) system so maybe we should go only with that two flags and reuse them with appropriate language 🤔

@magicmatatjahu makes sense to me 👍

Regarding the other suggestions and discussions, I have no preference here, ping me once you reached a consensus on what needs to change 🙂 Then I apply all above mentioned changes in one go.

@magicmatatjahu

Copy link
Copy Markdown
Member

I don't want this to be too late, but haven't you thought about being compatible with the cli generator, which we will move to this repository? E.g. the parameters for template are given by flags -p giving the name and value. Maybe we could go the same way for modelina, so "shape" of generate types command should looks like:

asyncapi generate types <asyncapi-file> <language> -p <name=value>

We can also add JSON Schemas (in the modelina repo) for available parameters to the given language and we can validate these parameters by AJV. Some benefits by this solution:

  • we will update available options in the modelina repo, and after release we will have that new options in the cli without having to contribute here. We will also avoid unnecessary flags in that command, everything will be inside modelina repo and due to fact that we have automatic bumps dependant packages, we will always up to date with modelin in cli :)
  • we can also add available languages list in the modelina so we can also avoid contributions to the cli when we will add new language

What do you think? @jonaslagoni

@jonaslagoni

Copy link
Copy Markdown
Member Author

I have tried to move along with the suggestion, but current this is out of reach until we solve some serious problems with JSON Schema dereferencing tools.

So for now, we have to hardcode it.

@jonaslagoni

jonaslagoni commented Feb 9, 2022

Copy link
Copy Markdown
Member Author

There is a large discrepancy between how files are provided to commands, some use fileName others file. I leave it up to you to create that consistency and remain with the file as a flag to allow context switching.

Finished adapting all the suggestions. New CLI examples:

asyncapi generate types typescript --file='./asyncapi.json' --output='./output'
asyncapi generate types javascript --file='./asyncapi.json' --output='./output'
asyncapi generate types java --file='./asyncapi.json' --packageName='my.custom.package' --output='./src/my/custom/package'
asyncapi generate types csharp --file='./asyncapi.json' --namespace='my.custom.namespace' --output='./src/my/custom/namespace'
asyncapi generate types golang --file='./asyncapi.json' --packageName='my.custom.package' --output='./src/my/custom/package'

I also wonder if that long goPackageName, javaPackageName or csharpNamespace flags are needed. I know this is because of the different types of modules in the languages, but maybe packageName and namespace is enough? Another languages have namespace or module (packages) system so maybe we should go only with that two flags and reuse them with appropriate language 🤔

I like it, changed 👍

I also wonder if we should make as followup flag to load JS file to load config for given Langauge Generator? As you know user has possibility to change format of model name etc. and of course that config we cannot convert to the CLI flags/parameters. WDYT?

Another feature, that Modelina natively should support at some point, agree (similar to tsconfig.json) 👍

derberg
derberg previously approved these changes Jun 28, 2022

@derberg derberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 🚀

@Souvikns @magicmatatjahu

Do you also approve? You were involved here so want to make sure you are fine too

/dnm

Souvikns
Souvikns previously approved these changes Jun 29, 2022

@Souvikns Souvikns left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍🏼

magicmatatjahu
magicmatatjahu previously approved these changes Jun 29, 2022

@magicmatatjahu magicmatatjahu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One comment: Should we change generate types to the generate models?

@derberg

derberg commented Jun 29, 2022

Copy link
Copy Markdown
Member

@jonaslagoni some conflicts popped up after the last merge.

One comment: Should we change generate types to the generate models?

I do not have an opinion, we can support both 😄

# Conflicts:
#	README.md
#	package-lock.json
#	package.json
@jonaslagoni jonaslagoni dismissed stale reviews from magicmatatjahu, Souvikns, and derberg via 68f3fe4 June 29, 2022 11:15
@jonaslagoni

Copy link
Copy Markdown
Member Author

One comment: Should we change generate types to the generate models?

I like generate models better, but types are fine as well 🤷 So no strong opinion.

@jonaslagoni some conflicts popped up after the last merge.

Fixed.

@magicmatatjahu

Copy link
Copy Markdown
Member

So generate models 😄

@jonaslagoni

Copy link
Copy Markdown
Member Author

So generate models 😄

@Souvikns @derberg what do you think? Stay or switch?

@derberg derberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

models is fine 🤷🏼

@Souvikns

Copy link
Copy Markdown
Member

models makes more sense 🤔

@jonaslagoni

Copy link
Copy Markdown
Member Author

Alright, switched to models 👍

@sonarqubecloud

Copy link
Copy Markdown

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

@derberg

derberg commented Jun 29, 2022

Copy link
Copy Markdown
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 426527f into asyncapi:master Jun 29, 2022
@asyncapi-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 0.21.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.

Integrate modelina to generate models

7 participants