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 modelina to generate models #173

Merged
merged 33 commits into from
Jun 29, 2022

Conversation

jonaslagoni
Copy link
Member

@jonaslagoni jonaslagoni commented Jan 5, 2022

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

@jonaslagoni jonaslagoni marked this pull request as ready for review January 11, 2022 12:04
@jonaslagoni
Copy link
Member Author

PR is ready to be reviewed 🙂

@derberg
Copy link
Member

derberg commented Jan 12, 2022

@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
Member

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

@derberg
Copy link
Member

derberg commented Jan 12, 2022

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

@fmvilas
Copy link
Member

fmvilas commented Jan 12, 2022

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
Copy link
Member

magicmatatjahu commented Jan 13, 2022

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
Copy link
Member

derberg commented Jan 13, 2022

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

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Jan 18, 2022

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
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
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
Copy link
Member Author

jonaslagoni commented Feb 9, 2022

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
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 🚀

@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
Copy link
Member

@Souvikns Souvikns left a comment

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
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.

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

@derberg
Copy link
Member

derberg commented Jun 29, 2022

@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
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
Member

So generate models 😄

@jonaslagoni
Copy link
Member Author

So generate models 😄

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

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.

models is fine 🤷🏼

@Souvikns
Copy link
Member

models makes more sense 🤔

@jonaslagoni
Copy link
Member Author

Alright, switched to models 👍

@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
0.0% 0.0% Duplication

@derberg
Copy link
Member

derberg commented Jun 29, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 426527f into asyncapi:master Jun 29, 2022
@asyncapi-bot
Copy link
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