Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_lsp): read configuration file #2872

Merged
merged 9 commits into from
Jul 15, 2022
Merged

Conversation

ematipico
Copy link
Contributor

@ematipico ematipico commented Jul 13, 2022

Summary

Closes #2861

This PR adds a feature to read the configuration rome.json file from the LSP. In order to do so, the Session has been extended with two new properties:

  • file_system, the trait that we use to read files
  • configuration, where we save the configuration read from the file

There's an intentional limitation here, which is if the a configuration file is found and it doesn't throw any errors, the workspace settings will use all the settings coming from the configuration.

This means that if we both have a configuration file with lineWidth set to 120, and then we have also a custom line width in the VSCode extension set to 160, the LSP will use the one inside the configuration.

Another intentional limitation is the fact that if we change the rome.json, the LSP won't pick the new change. In order to watch the change, we would need to enable the extension also for JSON language, but doing so would also enable the formatting for JSON files. Trying to format a JSON file, now, would emit a modal error inside VSCode. Not sure if that's what we want, but for now I left it out of scope.

Test Plan

I manually created rome.json config file and opened a project.

{
  "root": true,
  "formatter": {
    "lineWidth": 50
  }
}

Here's the logging:

├─343ms TRACE The LSP will now use the following configuration: 
│  WorkspaceSettings { format: FormatSettings { enabled: true, format_with_errors: false, indent_style: Some(Tab), line_width: Some(LineWidth(50)) }, linter: LinterSettings { enabled: true }, languages: LanguagesSettings { javascript: LanguageSettings { format: JsFormatSettings { indent_style: None, line_width: None, quote_style: Some(Double) } } } }

And when formatting a file, here's a piece of snippet:

const { getStdin } = require(
	"../common/third-party.js",
);

@ematipico ematipico changed the title feat(rome_cli): read configuration file feat(rome_cli): read configuration file from LSP Jul 13, 2022
@ematipico ematipico changed the title feat(rome_cli): read configuration file from LSP feat(rome_lsp): read configuration file Jul 13, 2022
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 13, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4a1cdf9
Status:🚫  Build failed.

View logs

@ematipico ematipico force-pushed the feature/load-configuration-lsp branch from 1a97b28 to 3c70fbb Compare July 13, 2022 14:23
@ematipico ematipico marked this pull request as ready for review July 13, 2022 16:19
Base automatically changed from feature/configuration-for-linter to main July 15, 2022 11:52
@ematipico ematipico force-pushed the feature/load-configuration-lsp branch from a415e4b to 4a1cdf9 Compare July 15, 2022 12:04
@ematipico ematipico temporarily deployed to aws July 15, 2022 12:04 Inactive
@github-actions
Copy link

@ematipico ematipico requested a review from leops July 15, 2022 12:16
Copy link
Contributor

@leops leops left a comment

Choose a reason for hiding this comment

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

Maybe we should consider deprecating loading the configuration from the editor settings. Would it be possible to emit a warning diagnostic in the vscode settings file if we detect a configuration section there ?

@ematipico
Copy link
Contributor Author

ematipico commented Jul 15, 2022

Maybe we should consider deprecating loading the configuration from the editor settings. Would it be possible to emit a warning diagnostic in the vscode settings file if we detect a configuration section there ?

VSCode marks the removed settings with warning (yellow) underline.

Those settings are marked as "BETA" in the VSCode interface. Maybe in the next release if the configuration file land and it's stable, we can just remove them directly. They were in BETA anyway. In the changelog we can add a paragraph as migration strategy.

@ematipico ematipico merged commit 332130c into main Jul 15, 2022
@ematipico ematipico deleted the feature/load-configuration-lsp branch July 15, 2022 15:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consume configuration via LSP
2 participants