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: create initial modelina cli #1785

Merged
merged 16 commits into from
Mar 1, 2024
Merged

Conversation

devilkiller-ag
Copy link
Member

Description

Create a CLI for Modelina and integrate it with AsyncAPI CLI

Related Issue

Related to issue #1724

Checklist

  • The code follows the project's coding standards and is properly linted (npm run lint).
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated to reflect the changes.
  • All tests pass successfully locally.(npm run test).

Copy link

netlify bot commented Feb 8, 2024

Deploy Preview for modelina canceled.

Name Link
🔨 Latest commit 4a4ecad
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/65df44e6f63640000829204b

@devilkiller-ag devilkiller-ag self-assigned this Feb 8, 2024
@devilkiller-ag
Copy link
Member Author

@jonaslagoni I have generated a CLI using OCLIF. I will be working next to add some basic commands.

@jonaslagoni
Copy link
Member

@devilkiller-ag you could just copy paste the command from the AsyncAPI CLI as a baseline ✌️

@devilkiller-ag
Copy link
Member Author

@devilkiller-ag you could just copy paste the command from the AsyncAPI CLI as a baseline ✌️

Okay Sure I will do that 👍🏼

@devilkiller-ag
Copy link
Member Author

Hey @jonaslagoni, Do I need to install modelina as a dependency in modelina-cli or should I generate these generators from the build of modelina?

import { CSharpFileGenerator, JavaFileGenerator, JavaScriptFileGenerator, TypeScriptFileGenerator, GoFileGenerator, Logger, DartFileGenerator, PythonFileGenerator, RustFileGenerator, TS_COMMON_PRESET, TS_JSONBINPACK_PRESET, CSHARP_DEFAULT_PRESET, CSHARP_NEWTONSOFT_SERIALIZER_PRESET, CSHARP_COMMON_PRESET, CSHARP_JSON_SERIALIZER_PRESET, KotlinFileGenerator, TS_DESCRIPTION_PRESET, PhpFileGenerator, CplusplusFileGenerator, JAVA_CONSTRAINTS_PRESET, JAVA_JACKSON_PRESET, JAVA_COMMON_PRESET, JAVA_DESCRIPTION_PRESET } from '@modelina/generators';

import type { AbstractGenerator, AbstractFileGenerator } from '@asyncapi/modelina';

@jonaslagoni
Copy link
Member

@devilkiller-ag I would say use dependency and use file://../ to link to the root directory, assuming you are under /modelina-cli

@devilkiller-ag
Copy link
Member Author

@jonaslagoni Okay I will do that.

@devilkiller-ag
Copy link
Member Author

devilkiller-ag commented Feb 14, 2024

use file://../ to link to the root directory, assuming you are under /modelina-cli

Hey @jonaslagoni, can you elaborate a little more about it? I am sorry I couldn't understand it. If we install modelina as a dependency in the modelina-cli project folder, we can use it directly. Then what's the need to import from the Modelina project root folder? I am sorry I am a little bit blurry with my thoughts.

@jonaslagoni
Copy link
Member

I think you are gonna run into problems when you have to pack and build the cli if you dont use dependency, but I might be wrong 🙂

So yea, do what ever is easiest and straight forward for you now, we can easily change it

@jonaslagoni
Copy link
Member

Hey @devilkiller-ag are you stuck on anything? 🙂

@devilkiller-ag
Copy link
Member Author

Hi @jonaslagoni, yeah I was stuck with some type errors that I can't find why they are happening. I got busy with my class assignments before I could further investigate. I will resume working on this tomorrow.

Here is the issues that I am currently facing:

  • In the file modelina-cli/src/parser.ts,
export function formatOutput(diagnostics: Diagnostic[], format: `${OutputFormat}`, failSeverity: SeveritytKind) {
  const options = { failSeverity: getDiagnosticSeverity(failSeverity) };
  switch (format) {
  case 'stylish': return stylish(diagnostics, options);
  case 'json': return json(diagnostics, options);
  case 'junit': return junit(diagnostics, options);
  case 'html': return html(diagnostics, options);
  case 'text': return text(diagnostics, options);
  case 'teamcity': return teamcity(diagnostics, options);
  case 'pretty': return pretty(diagnostics, options);
  default: return stylish(diagnostics, options);
  }
}

In this function, when we are passing options to any FormatterOption I get the following error:

Argument of type '{ failSeverity: -1 | DiagnosticSeverity; }' is not assignable to parameter of type 'FormatterOptions'.ts(2345)
const options: {
    failSeverity: -1 | DiagnosticSeverity;
}

When I checked the original code from asyncapi-cli, I got the same error there too. Do you have any hint about it?

@devilkiller-ag
Copy link
Member Author

I have pushed my latest code for your review

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Left a few comments, hope it unblocks you ✌️

Looks like you are nearly there 💪

modelina-cli/.github/dependabot.yml Outdated Show resolved Hide resolved
modelina-cli/CHANGELOG.md Outdated Show resolved Hide resolved
modelina-cli/src/commands/generate/models.ts Show resolved Hide resolved
modelina-cli/src/commands/generate/models.ts Outdated Show resolved Hide resolved
modelina-cli/src/parser.ts Outdated Show resolved Hide resolved
modelina-cli/tsconfig.json Outdated Show resolved Hide resolved
@devilkiller-ag
Copy link
Member Author

Hey @jonaslagoni, I made the changes you suggested, can you please checkout the code?

@devilkiller-ag
Copy link
Member Author

@jonaslagoni Do I need to add tests for cli?

@coveralls
Copy link

coveralls commented Feb 22, 2024

Pull Request Test Coverage Report for Build 8081649738

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.303%

Totals Coverage Status
Change from base Build 8061790627: 0.0%
Covered Lines: 5994
Relevant Lines: 6325

💛 - Coveralls

@devilkiller-ag
Copy link
Member Author

I have disabled some eslit rules as they were asking unnecessary changes in existing code

Copy link
Member

@jonaslagoni jonaslagoni 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 you need to add the modelina-cli to the list in the root .eslintignore otherwise it will try to run it there without the proper plugins.

Otherwise it looks fine to me 👌 Lets focus on getting a bare minimum working CLI here first and then we can experiment with embedding it into the AsyncAPI CLI 🙂

modelina-cli/README.md Outdated Show resolved Hide resolved
modelina-cli/src/commands/generate/index.ts Outdated Show resolved Hide resolved
@devilkiller-ag
Copy link
Member Author

Hi @jonaslagoni, can you please review this PR?

@devilkiller-ag
Copy link
Member Author

@jonaslagoni Do I need to resolve sonar cloud code duplication check fail?This check was passing before the merge from the master.

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Got some small changes that is needed to set this up better for release and running it locally. Basically just grabbing all the good stuff from the AsyncAPI CLI, no need to reinvent the wheel ✌️

  1. Grab all the files in https://github.com/asyncapi/cli/tree/master/bin
  2. Grab the following script: https://github.com/asyncapi/cli/blob/master/scripts/releasePackagesRename.js
  3. Grab the following script: https://github.com/asyncapi/cli/blob/master/scripts/updateUsageDocs.js
  4. Adapt the files which has suggestions
  5. Grab the AsyncAPI CLI testing setup: https://github.com/asyncapi/cli/blob/master/test/
  6. Take the Modelina test which can be found here (make sure to grab all related testing files): https://github.com/asyncapi/cli/blob/master/test/integration/generate/models.test.ts

modelina-cli/.eslintignore Outdated Show resolved Hide resolved
modelina-cli/.gitignore Outdated Show resolved Hide resolved
modelina-cli/package.json Show resolved Hide resolved
modelina-cli/tsconfig.json Show resolved Hide resolved
@jonaslagoni
Copy link
Member

@jonaslagoni Do I need to resolve sonar cloud code duplication check fail?This check was passing before the merge from the master.

No, completely ignore the sonarcloud ✌️

modelina-cli/dist/base.d.ts Outdated Show resolved Hide resolved

if (output) {
const models = await fileGenerator.generateToFiles(
document,
Copy link
Member Author

@devilkiller-ag devilkiller-ag Feb 28, 2024

Choose a reason for hiding this comment

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

Hi @jonaslagoni,
What should I pass in place of document here (Line no. 387 & 396):

const models = await fileGenerator.generateToFiles(
        document,
        output,
        { ...fileOptions, });

In Async API CLI, document was generated by the parser as

// Ref: Line no. 177-187 -> https://github.com/asyncapi/cli/blob/master/src/commands/generate/models.ts 
const { document, diagnostics ,status } = await parse(this, inputFile, flags);
    if (!document || status === 'invalid') {
      const severityErrors = diagnostics.filter((obj) => obj.severity === 0);
      this.log(`Input is not a correct AsyncAPI document so it cannot be processed.${formatOutput(severityErrors,'stylish','error')}`);
      return;
    }
    
    // Modelina, atm, is not using @asyncapi/parser@v3.x but @asyncapi/parser@v2.x, so it still uses Parser-API v1.0.0. 
    // This call converts the parsed document object using @asyncapi/parser@v3.x (Parser-API v2) to a document compatible with the Parser-API version in use in @asyncapi/parser@v2.x  (v1)
    // This is needed until https://github.com/asyncapi/modelina/issues/1493 gets fixed.
    const convertedDoc = ConvertDocumentParserAPIVersion(document.json(), 1);

And this convertedDoc was passed here in place of document:

const models = await fileGenerator.generateToFiles(
        convertedDoc as any,
        output,
        { ...fileOptions, });

But here we have removed the parser, so what should we use in place of this?

Copy link
Member

@jonaslagoni jonaslagoni Feb 28, 2024

Choose a reason for hiding this comment

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

The content of the file in question as a string 🙂

Copy link

sonarcloud bot commented Feb 28, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
4.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Nearly there 💪

@@ -0,0 +1,3 @@

Copy link
Member

Choose a reason for hiding this comment

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

You can add this to ignore list 🙂

@@ -0,0 +1,19 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

You can remove the entire minitemplate directory 🙂

@@ -0,0 +1,102 @@
import { expect, test } from '@oclif/test';
Copy link
Member

Choose a reason for hiding this comment

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

All of these non existing commands can just be removed completely 🙂 Only need the Modelina test and generic tests for context ✌️

@jonaslagoni jonaslagoni changed the base branch from master to cli March 1, 2024 08:53
@jonaslagoni
Copy link
Member

@devilkiller-ag as we discussed, I created a specific branch we can slowly work on, a bunch of good stuff in this PR so let's merge what we have, and I can do a little bit of cleanup 💪

@jonaslagoni
Copy link
Member

@devilkiller-ag do you mind creating a draft PR that targets master from the cli branch 🙂 ?

@jonaslagoni jonaslagoni changed the title feat: create modelina cli feat: create initial modelina cli Mar 1, 2024
@jonaslagoni jonaslagoni merged commit 1f5703a into asyncapi:cli Mar 1, 2024
17 of 21 checks passed
@devilkiller-ag
Copy link
Member Author

@devilkiller-ag do you mind creating a draft PR that targets master from the cli branch 🙂 ?

Okay sure

@devilkiller-ag devilkiller-ag deleted the cli branch March 2, 2024 09:15
@devilkiller-ag devilkiller-ag mentioned this pull request Mar 2, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants