tools icon indicating copy to clipboard operation
tools copied to clipboard

🐛 The VSCode Extension should not revert all settings back to default on benign configuration issues

Open nstepien opened this issue 2 years ago • 3 comments

Environment information

OS: win11, 64bit
Installation method: Release version of the extension in VSCode

What happened?

  1. Add a benign configuration mistakes in rome.json like
    {
      "javascript": {
        "formatter": {
          "quoteStyle": "single",
          "test": "test"
        }
      }
    }
    
  2. Reload window
  3. Trigger vscode to format a file with Rome

Expected result

Rome should ignore benign mistakes, and honor the otherwise valid settings. Instead it reverts everything to Rome's default settings.

I've added javascript.formatter.trailingComma: none in my config, but since the Release version of the extension does not support it, it just breaks the whole config. This can also be an issue if, for example, my locally installed version of the extension is outdated and does not support new settings. Another example would be if Rome renames/removes/moves a setting/rule, then the whole config breaks in VSCode.

Config issues should definitely be reported, at least with the CLI, but should not break everything.

Code of Conduct

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

nstepien avatar Oct 26 '22 21:10 nstepien

There's a few things to unpack here: The root cause of the issue is how serde fundamentally works, since we are deserializing the configuration into strongly typed Rust structs the data has to strictly adhere to the expected layout, or any error no matter how small will cause the entire file to fail parsing. We should try to recover from these errors instead, it would be possible to some extent while keeping serde if we first deserialized the configuration into a serde_json::Value then visit the resulting data to build up our Rust-based representation while checking for issues and manually handling them. For more advanced error recovery though this will have to wait until we implement our own JSON parsing. The second point is that the LSP should not consider a failure to load the configuration as equivalent to the configuration being absent. It's okay to load the default configuration if no rome.json file was found, but if the file does exists and has errors the only safe thing to do is returning an error on all linting and formatting operations since we can't be sure they are running with the correct configuration. Finally we could emit diagnostics for the configuration file ourselves instead of relying on the JSON schema validation in VS Code, this would allow us to flag the file as having errors immediately on workspace initialization, instead of having the errors being detect only once the user opens their rome.json file

leops avatar Oct 27 '22 07:10 leops

(@MichaReiser did you mean to swap the "unconfirmed" label with the "confirmed" label?)

nstepien avatar Oct 28 '22 11:10 nstepien

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

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

One issue I encounter from time to time is the extension reverting back to defaults because I checked out an older branch that has a config for Rome 10, while the extension expects (or runs?) Rome 11.

nstepien avatar Dec 12 '22 19:12 nstepien

@ematipico I think this isn't an issue anymore, should we close this?

nstepien avatar May 06 '23 00:05 nstepien