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

Initial prototype implementation #8

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

msujew
Copy link

@msujew msujew commented Sep 4, 2024

This is an initial implementation of the PL/I language server using Langium. It supports most (as far as we can tell) of the base language. Missing prominent features:

  • Macro language (preprocessor)
  • Compiler flags

packages/language/src/parser/pli-grammar.ts Outdated Show resolved Hide resolved
scripts/merge-tmlanguage.mjs Outdated Show resolved Hide resolved
@asatklichov asatklichov self-assigned this Sep 4, 2024
@msujew msujew force-pushed the feature/langium-project branch from b06d804 to 9b0ecae Compare September 4, 2024 13:03
Copy link

@asatklichov asatklichov left a comment

Choose a reason for hiding this comment

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

Code is nice 👍, now will check tests and do some testing

@@ -0,0 +1,34 @@
{
Copy link

@asatklichov asatklichov Sep 5, 2024

Choose a reason for hiding this comment

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

Just note: Some versions seems older, seen by npm outdated, and npm install shows some 'moderate severity vulnerability' but build becomes OK once I run npm update

};
}

readFile(uri: vscode.Uri): Uint8Array {

Choose a reason for hiding this comment

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

unused 'uri' but can be replaced with _uri, same for stat()

packages/language/src/parser/pli-lexer.ts Show resolved Hide resolved
deps: {
interopDefault: true
},
include: ['packages/**/test/**/*.test.ts']
Copy link

@asatklichov asatklichov Sep 5, 2024

Choose a reason for hiding this comment

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

Can we set some basic coverageThreshold in vitest config? Build automatically fails if coverage drops down, simple and helpful during dev.
Like in Jest e.g.:
thresholds: {
lines: 80,
functions: 80,
branches: 80,
statements: 80
}

Choose a reason for hiding this comment

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

I would strongly advise against that because it won't help with the quality and will create a number of undesired side effects.

Choose a reason for hiding this comment

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

This is common practice. Side effect only happens in code change not in test, this is just a test coverage measurement. Same measures tuned in sonar or Jenkins upon project.

@KUGDev KUGDev self-requested a review September 5, 2024 18:36
@KUGDev
Copy link
Contributor

KUGDev commented Sep 6, 2024

LGTM, works the same way in IntelliJ IDEA

@VitGottwald
Copy link

Please, add .nvmrc file with expected node version (e.g. v20.17.0).

Comment on lines 1 to 3
.vscode/**
.vscode-test/**
src/**

Choose a reason for hiding this comment

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

How about explicitly only including what is needed? package.json and README.md are included automatically.

Suggested change
.vscode/**
.vscode-test/**
src/**
# Ignore everything
*
**/*
../**/* # when `vsce` run with `--no-dependencies` this is not needed
../../**/* # when `vsce` run with `--no-dependencies` this is not needed
# Add what we actually want
!out/extension/main.cjs
!out/extension/main.cjs.map
!language-configuration.json
!syntaxes/pli.merged.json

.vscode/launch.json Outdated Show resolved Hide resolved
@VitGottwald
Copy link

Can we replace pl-one everywhere in the code and package.json(s) with pli to have single word language name like we do for the other extensions (hlasm, cobol, rexx, jcl, ...)?

code_samples/CHART.pli Outdated Show resolved Hide resolved
code_samples/MACROS.pli Outdated Show resolved Hide resolved
@asatklichov
Copy link

  • adding CI build, vsix gen, and browser exec
  • dummy validation and semantic class

@msujew msujew force-pushed the feature/langium-project branch 7 times, most recently from 8aa2a54 to a6e1b12 Compare November 18, 2024 13:20

Choose a reason for hiding this comment

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

Let's leave an empty line - MD047

Choose a reason for hiding this comment

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

New EOL character MD047

@KUGDev KUGDev self-requested a review November 21, 2024 12:14
@KUGDev
Copy link
Contributor

KUGDev commented Nov 21, 2024

From the repo perspective, it would be better to separate the actual solution between different folders. The implementation is intended to be supported both in VS Code and IntelliJ IDEA. Can I ask the team to bring all the VS Code related things into the "vscode-extension" folder? So that it would be a clear separation between the VS Code and IntelliJ IDEA parts in the future

@azatsatklichov
Copy link

From the repo perspective, it would be better to separate the actual solution between different folders. The implementation is intended to be supported both in VS Code and IntelliJ IDEA. Can I ask the team to bring all the VS Code related things into the "vscode-extension" folder? So that it would be a clear separation between the VS Code and IntelliJ IDEA parts in the future

It is a good idea, I agree.

@msujew
Copy link
Author

msujew commented Nov 21, 2024

Can I ask the team to bring all the VS Code related things into the "vscode-extension" folder? So that it would be a clear separation between the VS Code and IntelliJ IDEA parts in the future

@KUGDev I agree, that's why all the VS Code related parts already are part of the packages/extension directory. Are there any specific parts we've missed?

@KUGDev
Copy link
Contributor

KUGDev commented Nov 21, 2024

@msujew ok I see. Then the IntelliJ IDEA related stuff will go into the "packages" folder as well. Will the "extension" folder remain with the same name? Cause if we add the "plugin" folder, it would mean that we are going to provide IntelliJ IDEA plugin only. I guess, the "intellij-plugin" naming would be more appropriate, and to stay consistent, it would mean that for the VSCode extension the folder name "vscode-extension" would be more appropriate as well

@msujew msujew force-pushed the feature/langium-project branch 2 times, most recently from cfe3f94 to d5bf1a6 Compare November 21, 2024 16:23
Copy link

@azatsatklichov azatsatklichov left a comment

Choose a reason for hiding this comment

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

💯

@msujew msujew force-pushed the feature/langium-project branch 2 times, most recently from 1f0d1c2 to cfcd5bf Compare November 26, 2024 17:37
},
"volta": {
"node": "18.20.3",
"npm": "10.7.0"

Choose a reason for hiding this comment

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

What about pnpm, can Volta manage it as well?

Also how about having an .nvmrc file? I think it is more common than Volta. In addition to nvm it is also supported by fnm which I have been successfully using and very happy with for years.

Copy link
Author

Choose a reason for hiding this comment

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

I believe our yeoman generator automatically adds the volta entry. I can add a .nvmrc file instead 👍

Co-authored-by: Benjamin Wilson <[email protected]>
Co-authored-by: Markus Rudolph <[email protected]>
Signed-off-by: Mark Sujew <[email protected]>
@msujew msujew force-pushed the feature/langium-project branch from cfcd5bf to 6b5d434 Compare December 11, 2024 13:19
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.

6 participants