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

feat(editors/vscode): Add requiresConfiguration option #4023

Merged
merged 4 commits into from
Dec 23, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Dec 9, 2022

Summary

This PR introduces a new rome.requiresConfiguration option that disables the linter and formatter if a workspace doesn't contain a rome.json configuration.

The addition of the option is motivated by #3506 where users want an easy way to disable Rome for all projects not using Rome.

The new option doesn't fully disable Rome but only disables the linter and formatter (operations performed automatically on save). This has the benefit that other actions like sorting imports or refactors remain available.

The implementation for the linter and formatter differ:

  • Formatter: I changed the extension to not register the formatting capabilities if the rome.json file is missing and the requiresConfiguration option is set. This allows the user to e.g. use the default VS Code formatter instead and avoids the situation where a user triggers "Format Document" and nothing happens.
  • Linter: Returns an empty list of diagnostics from pull_diagnostic

Test Plan

I tested that:

  • Formatting and linting is working if the option is false
  • Formatting and linting is working if the option is true and a rome.json file is present
  • Formatting and linting is disabled if the option is true and the project has no rome.json file

@MichaReiser MichaReiser added the A-Editors Area: editors label Dec 9, 2022
@MichaReiser MichaReiser added this to the Next milestone Dec 9, 2022
@MichaReiser MichaReiser requested review from ematipico and a team as code owners December 9, 2022 15:46
@netlify
Copy link

netlify bot commented Dec 9, 2022

Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 0a8ec9a
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/63a5822ddc54d200086b21e1

@MichaReiser MichaReiser requested a review from a team December 9, 2022 17:27
@leops
Copy link
Contributor

leops commented Dec 21, 2022

It looks like the merge conflict with #4044 runs a bit deeper than a simple text-based merge, I can look into properly rebasing this branch as this new feature could be used to improve the way the language server handles configuration loading errors (by also disabling linting and formatting when the configuration file fails to load)

This PR introduces a new `rome.requiresConfiguration` option that disables the linter and formatter if a workspace doesn't contain a `rome.json` configuration.

The addition of the option is motivated by #3506 where users want to have an easy way to disable Rome for all projects not using Rome.

The new option doesn't fully disable Rome but only disables the linter and formatter (operations that are performed automatically on save). This has the benefit that other actions like sorting imports, or refactors remain available.

The implementation for the linter and formatter differ:

* Formatter: I changed the extension to not register the formatting capabilities if the `rome.json` file is missing and the `requiresConfiguration` option is set. This so that VS Code can pick up another default formatter (if installed).
* Linter: Returns an empty list of diagnostics from `pull_diagnostic`

I tested that:
* Formatting and linting is working if the option is false
* Formatting and linting is working if the option is `true` and a `rome.json` file is present
* Formatting and linting is disabled if the option is `true` and the project has no `rome.json` file
@leops leops force-pushed the feat/requires-configuration branch from 39a82c0 to 75a66c7 Compare December 21, 2022 14:42
@leops
Copy link
Contributor

leops commented Dec 21, 2022

I've rebased the branch on main, and included a small refactor on the Session to allow the feature introduced in this PR to continue working after #4044 has been merged and the content of the configuration file is no longer kept in memory in the session: the code responsible for loading the configuration now writes to a configuration_status field on the Session that reflects whether a configuration was loaded correctly, has failed to parse or did not exist. This status is used in the new is_linting_and_formatting_disabled method to disable linting and formatting if the configuration failed to load, or if no configuration file exists and the requiresConfiguration option has been set.

crates/rome_lsp/src/session.rs Outdated Show resolved Hide resolved
@ematipico
Copy link
Contributor

Does the PR require updating some documentation around the repository/website?

@MichaReiser
Copy link
Contributor Author

I've rebased the branch on main, and included a small refactor on the Session to allow the feature introduced in this PR to continue working after #4044 has been merged and the content of the configuration file is no longer kept in memory in the session: the code responsible for loading the configuration now writes to a configuration_status field on the Session that reflects whether a configuration was loaded correctly, has failed to parse or did not exist. This status is used in the new is_linting_and_formatting_disabled method to disable linting and formatting if the configuration failed to load, or if no configuration file exists and the requiresConfiguration option has been set.

Thanks @leops Let me clarify the description of the configuration before merging and mention the minimal required rome version

@MichaReiser
Copy link
Contributor Author

Does the PR require updating some documentation around the repository/website?

I can add a paragraph to the editor README

@MichaReiser
Copy link
Contributor Author

MichaReiser commented Dec 22, 2022

Patch I'm no longer able to commit to this PR. Here are the documentation changes:
Subject: [PATCH] Improve documentation
---
Index: editors/vscode/README.md
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/editors/vscode/README.md b/editors/vscode/README.md
--- a/editors/vscode/README.md	(revision 75a66c74e698c9464e706fe9b82edaebb8c06e21)
+++ b/editors/vscode/README.md	(revision 0d244a5b684f92f7687cbe450516fbae7df0771d)
@@ -88,6 +88,10 @@
 
 Enables Rome to handle renames in the workspace (experimental).
 
+### `rome.requireConfiguration`
+
+Disables formatting, linting, and syntax errors for projects without a `rome.json` file. Requires Rome 12 or newer.
+
 ## Known limitations
 
 ### Configuration per sub-directory
@@ -100,10 +104,6 @@
 
 ### Disable Rome for workspaces without a `rome.json` configuration
 
-Rome's VS Code extension is active for every workspace regardless if the workspace contains a `rome.json` configuration ([issue 3506](https://github.com/rome/tools/issues/3506)). That may be surprising to you if you use other extensions like ESLint where the extension is disabled if the configuration is missing. This behavior is intentional because it's Rome's philosophy that the configuration should be optional.
+You can set Rome's [`rome.requireConfiguration`](#romerequireconfiguration) setting to `true` to disable Rome's formatter, linter, and syntax errors for projects without a `rome.json` file.
 
-You can work around this limitation by [disabling Rome per workspace](https://code.visualstudio.com/docs/editor/extension-marketplace#_disable-an-extension):
 
-- Open the _Extensions_ panel
-- Search for Rome
-- Right-click on _Rome_ and select _Disable (Workspace)_
Index: editors/vscode/package.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/editors/vscode/package.json b/editors/vscode/package.json
--- a/editors/vscode/package.json	(revision 75a66c74e698c9464e706fe9b82edaebb8c06e21)
+++ b/editors/vscode/package.json	(revision 0d244a5b684f92f7687cbe450516fbae7df0771d)
@@ -107,7 +107,7 @@
 				"rome.requireConfiguration": {
 					"type": "boolean",
 					"default": false,
-					"markdownDescription": "Require a Rome configuration file to enable formatting and linting."
+					"markdownDescription": "Require a Rome configuration file to enable syntax errors, formatting and linting. Requires Rome 12 or newer."
 				}
 			}
 		},

I found another issue that must be solved before landing. Toggling the setting multiple times results in the rome formatter being registered multiple times.

grafik

@MichaReiser
Copy link
Contributor Author

A very collaborative PR :). Looks good to me. @leops you can hit the merge button when you're ready.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Editors Area: editors
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants