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

Configurable folder excludes #519

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Configurable folder excludes #519

merged 1 commit into from
Sep 5, 2022

Conversation

dhuebner
Copy link
Contributor

@dhuebner dhuebner commented Jun 2, 2022

Introduces a new configuration service.
Configuration service listens for changes of all registered Langium languages. Configurations are updated when user changes the settings (user/workspace). Other services can request configurations using:
getConfiguration(language: string, configuration: string)

Each language will have his own settings section, section name is the <languageId>-settings.

Exclude pattern syntax is handled by node-ignore module kaelzhang/node-ignore

For Langium grammar language it allows configuration of excluded folders using vscode settings.

Fixes #344

@dhuebner dhuebner marked this pull request as ready for review July 1, 2022 14:59
@dhuebner dhuebner changed the title WIP Configurable folder excludes #344 Configurable folder excludes Jul 15, 2022
@dhuebner dhuebner force-pushed the dh/exclude-settings-344 branch 2 times, most recently from 77a03ed to 3f554ca Compare July 15, 2022 14:48
@dhuebner dhuebner force-pushed the dh/exclude-settings-344 branch 3 times, most recently from 17f29ae to 08ce8d0 Compare July 29, 2022 07:29
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, thanks! I have a few proposals for reorganizing.

packages/langium/package.json Outdated Show resolved Hide resolved
packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
packages/langium/src/lsp/language-server.ts Outdated Show resolved Hide resolved
if (parentWsFolder) {
// create path relative to workspace folder root: /user/foo/workspace/entry.txt -> entry.txt
const wsFolderPath = URI.parse(parentWsFolder).path + '/';
entryPath = entryPath.replace(wsFolderPath, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

The string operations startsWith and replace seem unsafe to me: they can lead to false matches. Following my recommendation of moving this file to sprotty-vscode, you can use the Node.js path module for path operations.

@dhuebner dhuebner force-pushed the dh/exclude-settings-344 branch 3 times, most recently from 4bc41ef to 04eec5d Compare August 19, 2022 12:56
@msujew
Copy link
Member

msujew commented Aug 19, 2022

@dhuebner There's a compilation error in your latest changes

@dhuebner dhuebner force-pushed the dh/exclude-settings-344 branch 2 times, most recently from 0630b5d to 0d6d2a7 Compare August 19, 2022 13:50
@dhuebner
Copy link
Contributor Author

@msujew
Yeah.. There were some other issues as well :-(

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.

Thanks for the restructuring!

if (parentWsFolder) {
// create path relative to workspace folder root: /user/foo/workspace/entry.txt -> entry.txt
const wsFolderPath = URI.parse(parentWsFolder).path + '/';
entryPath = entryPath.replace(wsFolderPath, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

See my previous comment. Could you try using the path module instead of startsWith and replace string operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it doesn't matter which API we would use, we can't recover the missing information about the WorkspaceFolder we are operating on. That means that we can't create a WSFolder relative path.
I think the best is to pass over the WorkspaceFolder, I changed the implementation accordingly

packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
packages/langium/src/workspace/configuration.ts Outdated Show resolved Hide resolved
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.

👍

@dhuebner dhuebner merged commit 14eb532 into main Sep 5, 2022
@dhuebner dhuebner deleted the dh/exclude-settings-344 branch September 5, 2022 07:31
@spoenemann spoenemann added this to the v0.5.0 milestone Sep 15, 2022
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.

Introduce runtime language configuration and UI settings
3 participants