tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(editors/vscode): Add `requiresConfiguration` option

Open MichaReiser opened this issue 2 years ago • 1 comments

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 avatar Dec 09 '22 15:12 MichaReiser

Deploy Preview for docs-rometools canceled.

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

netlify[bot] avatar Dec 09 '22 15:12 netlify[bot]

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)

leops avatar Dec 21 '22 10:12 leops

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.

leops avatar Dec 21 '22 14:12 leops

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

ematipico avatar Dec 21 '22 15:12 ematipico

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 avatar Dec 21 '22 15:12 MichaReiser

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

I can add a paragraph to the editor README

MichaReiser avatar Dec 21 '22 15:12 MichaReiser

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 avatar Dec 22 '22 11:12 MichaReiser

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

MichaReiser avatar Dec 23 '22 10:12 MichaReiser