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

langium-cli to generate types definition #754

Merged
merged 1 commit into from
Dec 7, 2022
Merged

Conversation

dhuebner
Copy link
Contributor

@dhuebner dhuebner commented Nov 4, 2022

This PR adds:

  • a new option -t, --types <file> to langium-cli that only triggers the type definition generation
  • a command to the Langium extension that triggers langium-cli to generate type definitions
  • a context menu entry to the Langium editor the triggers the command

generate-langium-types

@dhuebner dhuebner force-pushed the dh/generate-types branch 2 times, most recently from ba03451 to 6638e5f Compare November 4, 2022 10:49
@dhuebner dhuebner marked this pull request as ready for review November 4, 2022 10:49
@pluralia
Copy link
Contributor

pluralia commented Nov 4, 2022

Could you please add "a button", that replaces returns keyword automatically and remove declared types and interfaces from the initial grammar?

Copy link
Contributor

@pluralia pluralia left a comment

Choose a reason for hiding this comment

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

Could you add also generation type files automatically for each .langium file in the workspace?

One more moment: the types generation is not correct, because of this issue. So, it's not a problem of your PR changes, but we should keep it in mind.

packages/langium-vscode/package.json Outdated Show resolved Hide resolved
log('Using langium-cli v' + String(cliVersion).trim());
} catch (error) {
// not installed
const installCmd = 'npm install -g langium-cli';
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't expect users to install the langium-cli globally. The extension should instead use the package that is in the node_modules. TypeScript for example works the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extension should instead use the package that is in the node_modules

I disagree here. Langium extension as part of the tooling should not use the project setup, but a compatible langium-cli version installed globally. Imagine you have a Langium project in v0.5.0 and Langium extension in v0.6.0, this will not work as the released v0.5.0 langium-cli lacks corresponding CLI command implementation.

However, I would love to see how TypeScript manages that with npm_modules folder, could you point me to the corresponding repository? We could offer this as an option before starting generation.

Btw. we suggest users to install example cli generators globally, is that something we can improve/change?

@msujew
Copy link
Member

msujew commented Nov 4, 2022

Could you add also generation type files automatically for each .langium file in the workspace?

What do you mean with that?

@pluralia
Copy link
Contributor

pluralia commented Nov 4, 2022

Could you add also generation type files automatically for each .langium file in the workspace?

What do you mean with that?

If I have some .langium files in my workspace and want to have a type file for each of them, currently, I have to start generation for each of them. I'd like to have a flag, that do it automatically.

@msujew
Copy link
Member

msujew commented Nov 4, 2022

@pluralia Since people will execute this command only rarely for their grammars, I'm fine with not generating all fines using a single click. We can add this additional feature in a separate PR, it's a bit out of scope for this PR.

@spoenemann
Copy link
Contributor

@dhuebner @pluralia please keep in mind that VS Code is designed to be terminal- and keyboard-centric. It's not like in Eclipse IDE where you need a menu item for everything. Adding UI elements can be perceived as intrusive by users who want to keep their IDE clean.

In fact I believe Eclipse IDE is a very bad example of what happens when all plug-in developers add their own UI elements: the tool becomes bloated with buttons, views and menu entries, and as result it feels heavyweight.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

I tried the following in examples/statemachine:

 $ ../../packages/langium-cli/bin/langium.js extract-types src/language-server/statemachine.langium 
Error: ENOENT: no such file or directory, open '/src/language-server/statemachine.langium'
    at Object.openSync (node:fs:590:3)
    at Object.readFileSync (node:fs:458:35)
    at NodeFileSystemProvider.readFileSync (/workspace/langium/packages/langium/lib/node/node-file-system-provider.js:22:29)
    at DefaultLangiumDocuments.getContent (/workspace/langium/packages/langium/lib/workspace/documents.js:180:40)
    at DefaultLangiumDocuments.getOrCreateDocument (/workspace/langium/packages/langium/lib/workspace/documents.js:174:70)
    at generateTypes (/workspace/langium/packages/langium-cli/lib/generate.js:206:32)
    at Command.<anonymous> (/workspace/langium/packages/langium-cli/lib/langium.js:38:34)
    at Command.listener [as _actionHandler] (/workspace/langium/node_modules/commander/lib/command.js:488:17)
    at /workspace/langium/node_modules/commander/lib/command.js:1227:65
    at Command._chainOrCall (/workspace/langium/node_modules/commander/lib/command.js:1144:12) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: '/src/language-server/statemachine.langium'
}

I guess you should resolve the input file against . (use path.resolve from Node.js), and check whether the file exists before opening it.

Second observation: Please check whether the output file exists before overwriting it. In that case I propose asking the user a yes/no question. You can use the readline Node.js module for this, see this example.

Third observation: The Langium editor does not support using types from other files yet: cross-references are broken and validations don't work as expected. Do we have open issues for that?

@dhuebner dhuebner added cli CLI related issue extension VS Code extension related issue labels Nov 18, 2022
@dhuebner
Copy link
Contributor Author

dhuebner commented Nov 18, 2022

@pluralia

Could you please add "a button", that replaces returns keyword automatically and remove declared types and interfaces from the initial grammar?

I think better would be to have a quick fix for that in the editor itself. Doing so we will also help users who are adding new types by hand. If we have such a quick fix, the extension could call this commands after generating is finished.

Could you please add "a button", that replaces returns keyword automatically and remove declared types and interfaces from the initial grammar?

Like already mentioned here, I think it's okay for now to start generation manually for each file. In cases where one need to generate many files one can write a script that calls langium extract-types for each file.

@dhuebner
Copy link
Contributor Author

@spoenemann

I guess you should resolve the input file against . (use path.resolve from Node.js), and check whether the file exists before opening it.

Done.

Second observation: Please check whether the output file exists before overwriting it. In that case I propose asking the user a yes/no question. You can use the readline Node.js module for this, see this example.

Done. Also added --force option to force overwrite existing file.

Third observation: The Langium editor does not support using types from other files yet: cross-references are broken and validations don't work as expected. Do we have open issues for that?

Yes, I think so.

@dhuebner
Copy link
Contributor Author

@dhuebner @pluralia please keep in mind that VS Code is designed to be terminal- and keyboard-centric.

New menu entry is only restricted to the Langium Grammar editor context, where it is definitely belongs to. I was hoping to create a Generate Langium later to start the Langium grammar generator. When/if we have that, we can group both menu entries into a Langium sub-menu.

In addition there is a Command (Shift+CMD+P) which is currently in Developer category, maybe we can create a 'Langium' category later.

}
},
"activationEvents": [
"onLanguage:langium"
"onLanguage:langium",
"langium-vscode.generate-types"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"langium-vscode.generate-types"
"onCommand:langium-vscode.generate-types"

@spoenemann
Copy link
Contributor

I'm skeptical about the UI contribution, both from a conceptual standpoint (#754 (comment)) and a technical standpoint (#754 (comment)). We shouldn't make too strong assumptions about the user's project setup. I doubt that we can make such a UI contribution work as the user expects in all circumstances (think of different package managers: yarn, pnpm, bun, ...)

@dhuebner please have a look at the Task Provider API, this could match our use case better than a command.

My proposal: Remove the UI contribution from this PR and create a separate PR for it after evaluating the Task Provider approach.

@dhuebner dhuebner changed the title Execute langium-cli using a command to generate types definition langium-cli to generate types definition Dec 1, 2022
@dhuebner
Copy link
Contributor Author

dhuebner commented Dec 1, 2022

My proposal: Remove the UI contribution from this PR and create a separate PR for it after evaluating the Task Provider approach.

@spoenemann
I removed all the UI contributions. Will look into Task API to check if it fits the requirements well and create a new PR.
The new cli functionality is very helpful also without UI, so please review once again :)

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Great! The only thing I'm missing is a console output after generating the types file, printing the path of the generated file so I can cmd-click it to open it right away.

Follow-up work: add an option to modify the grammar file so it uses the generated types instead of inferring them. @dhuebner could you create an issue for that?

Further follow-up work: #385

@dhuebner
Copy link
Contributor Author

dhuebner commented Dec 7, 2022

@spoenemann

The only thing I'm missing is a console output after generating the types file,

I added a log entry containing the generated file path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli CLI related issue extension VS Code extension related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants