tools icon indicating copy to clipboard operation
tools copied to clipboard

🐛 The VSCode Extension should not run in workspaces not using Rome

Open nstepien opened this issue 2 years ago • 17 comments

Environment information

OS: win11, 64bit

What happened?

  1. Install the VSCode extension
  2. Open a workspace not using Rome
  3. Edit a file to trigger a Rome lint issue

Expected result

I shouldn't see red squiggles coming from Rome. If a project does not use Rome, it doesn't mean the recommended Rome lint rules must necessarily apply to it.

I guess the extension could check for a rome.json in the root directory, or whether the npm package is installed.

Right now I have to manually disable/enable the extension per workspace. I can see some of my coworkers getting confused seeing errors in files that are otherwise OK per the project's current configuration.

Code of Conduct

  • [X] I agree to follow Rome's Code of Conduct

nstepien avatar Oct 26 '22 21:10 nstepien

Duplicate of #3271

leops avatar Oct 27 '22 07:10 leops

I shouldn't see red squiggles coming from Rome. If a project does not use Rome, it doesn't mean the recommended Rome lint rules must necessarily apply to it.

Relevant answer

ematipico avatar Oct 27 '22 08:10 ematipico

This issue is stale because it has been open 14 days with no activity.

github-actions[bot] avatar Nov 10 '22 12:11 github-actions[bot]

I don't think it's stale 🥹

nstepien avatar Nov 10 '22 12:11 nstepien

I suppose the issue comes from the fact that people are accustomed to prettier and eslint, which work via configuration or with the need to opt-in the single features.

Rome has a different philosophy, and the extension works in a different way.

The behaviour is intended and I wouldn't flag it as a bug. We are aware of the limitation, and in order to make this right we have to do some structural changes to our architecture, which are not yet planned.

ematipico avatar Nov 14 '22 11:11 ematipico

I would definitely say it's a bug. I have 20+ different workspaces of projects that aren't entirely dead and have their own git repo with a package.json in them. Only one of these uses rome (so far! :) ) If Rome gets enabled when I'm in one of the other 19 workspaces, how can it possibly know which binary to use (i.e. which version of rome) and what configuration it's supposed to use? (I know the answer, but that's not the point) I think it's implicit that the configuration comes from some vague default. Deriving it from the workspace's rome.json (and version from ./node_mobules/.bin/rome) would be explicit.

peterbe avatar Nov 15 '22 13:11 peterbe

For additional context, the Rome VSCode extension currently works on the same model as the built-in TypeScript extension. If you open a standalone .ts file in VSCode, even if it the workspace has no tsconfig.json file and no associated TypeScript version, the file will still get checked and show TS diagnostics using the default configuration options and the version of TypeScript that's bundled with the editor. I think the main difference here is that the default configuration for TypeScript is a lot more permissive than the one Rome is using: in TS the checkJs and strict settings are off by default, meaning the language server will only report syntax errors in JS files and non-strict typechecking errors in TS files, while Rome enables all the recommended lint rules by default and thus reports a lot more diagnostics.

leops avatar Nov 16 '22 13:11 leops

while Rome enables all the recommended lint rules by default and thus reports a lot more diagnostics.

Rome's recommended rules are also very opinionated IMO, I had to disable some of them as I found them counter-productive or not valuable.

nstepien avatar Nov 16 '22 13:11 nstepien

while Rome enables all the recommended lint rules by default and thus reports a lot more diagnostics.

Rome's recommended rules are also very opinionated IMO, I had to disable some of them as I found them counter-productive or not valuable.

It would be great you could open a discussion and explain which rules seem to make more harm than good. We're open to change the recommendation rule set based on real usages, things that us - unfortunately - lack.

ematipico avatar Nov 16 '22 15:11 ematipico

@ematipico Here you go: https://github.com/rome/tools/discussions/3770

nstepien avatar Nov 16 '22 23:11 nstepien

non-strict typechecking errors in TS files, while Rome enables all the recommended lint rules by default and thus reports a lot more diagnostics.

I think this is what triggered me to come here and share. I opened another project, that doesn't use Rome (yet) after recently having installed the VS Code extension and now I suddenly get a rest squiggly lines about if statements that don't have {...} wrapping. The redness is too distracting to ignore.

peterbe avatar Nov 17 '22 14:11 peterbe

For additional context, the Rome VSCode extension currently works on the same model as the built-in TypeScript extension. If you open a standalone .ts file in VSCode, even if it the workspace has no tsconfig.json file and no associated TypeScript version, the file will still get checked and show TS diagnostics using the default configuration options and the version of TypeScript that's bundled with the editor. I think the main difference here is that the default configuration for TypeScript is a lot more permissive than the one Rome is using: in TS the checkJs and strict settings are off by default, meaning the language server will only report syntax errors in JS files and non-strict typechecking errors in TS files, while Rome enables all the recommended lint rules by default and thus reports a lot more diagnostics.

Should we review our defaults when it comes to the language server? And have two different defaults: one for the CLI and one for the LSP.

ematipico avatar Nov 17 '22 17:11 ematipico

I see multiple options to allow users to customize rome to their liking

  • Add an option to disable the extension if a project has no rome.json file
  • Add an option to disable the formatter/linter
  • Add an option to specify a fallback rome configuration for projects that have no rome.json
  • add support for a user wide rome configuration in there home folder

I don't recommend using different recommended rules for the editor. This would be rather confusing

MichaReiser avatar Nov 17 '22 21:11 MichaReiser

I don't recommend using different recommended rules for the editor. This would be rather confusing

I think the idea would be more to have different default configurations in different context, for instance to have recommended rules enabled by default in the CLI but disabled by default in the language server

leops avatar Nov 18 '22 12:11 leops

I think the idea would be more to have different default configurations in different context, for instance to have recommended rules enabled by default in the CLI but disabled by default in the language server

I think this will be confusing because the CLI (and CI) will report more errors than the editor integration, forcing everyone to use a configuration in projects that they want to use Rome.

MichaReiser avatar Nov 18 '22 12:11 MichaReiser

"rome.exclude" would be nice.

"rome.exclude": {
	"app/project1": true,
	"app/project2": true,
},

Since they are include all by default.

NikolaRHristov avatar Nov 18 '22 13:11 NikolaRHristov

  • Add an option to disable the extension if a project has no rome.json file
  • add support for a user wide rome configuration in there home folder

I would really love to see support for these two options.

The first one is critical for anyone who works with projects that don't use Rome, and doesn't want to end up with tons of red squiggles and/or accidentally committing large formatting changes to other project.

The second is useful for running the formatter on legacy projects. I have a lot of old projects with 2-space indentation. If I don't watch like a hawk and toggle "save on format" manually when I switch projects, Rome automatically adds tabs to these projects, which causes various issues like throwing off git blame (unless you also remember to ignore revs, but the projects in question are not things I want to spend any more maintenance time on than needed). If Rome used 2-space indentation for such projects by default, it would be a lot less disruptive and I'd be happy to leave it on almost all the time.

"rome.exclude" would be nice.

"rome.exclude": {
	"app/project1": true,
	"app/project2": true,
},

Since they are include all by default.

Since Rome (like almost any other extension) works in GitHub Codespaces, I would advocate that it's important that any configuration can also apply to codespaces. (A conceptually straightforward solution might be a way to avoid running Rome in a codespace unless it's a recommended extension.)

lgarron avatar Dec 07 '22 20:12 lgarron

I know it's might be the most constructive ways to say this but; this here is why I'm not adopting Rome. The CLI is fantastic and I'm sick and tired of Prettier and ESlint being so slow. But I hate having to go in and disable the Rome VS Code extension every single time I hop over to another project. And when switching back to a side-project that uses Rome, I have to remember go reenable the extension.

I've been eyeing this issue periodically for progress and the consensus seems generally strong here. But some tangents about configuration options might prevent a quicker solution to get this unblocked.

Is there anything I can do to move this forward other than pestering? :)

peterbe avatar Feb 01 '23 14:02 peterbe

This has been implemented via https://github.com/rome/tools/pull/4023 and available via pre-release.

It will be available once we release the new version.

ematipico avatar Mar 19 '23 15:03 ematipico