vscode: simplified config and to removed one source of truth of default values #3131

Veetaha · 2020-02-13T20:50:42Z

Though not intended initially, the implementation of config design is alike dart's one as pointed by @matklad in PM.

…mplementation in mind

Veetaha · 2020-02-13T20:51:03Z

Saved 100+ lines of TypeScript code!

Veetaha · 2020-02-13T20:53:57Z

editors/code/src/config.ts

+    cargoWatchOptions(): CargoWatchOptions {
+        return {
+            enable:     this.cfg.get("cargo-watch.enable") as boolean,
+            arguments:  this.cfg.get("cargo-watch.arguments") as string[],
+            allTargets: this.cfg.get("cargo-watch.allTargets") as boolean,
+            command:    this.cfg.get("cargo-watch.command") as string,
+        };


Not sure entirely whether to keep the config flat and change this to 4 separate getter methods or not. Any opinions?

I like the nested variant more (a bit)

Veetaha · 2020-02-13T20:56:04Z

editors/code/src/config.ts

+
+    private refreshConfig() {
+        this.cfg = vscode.workspace.getConfiguration(Config.rootSection);
+        console.log("Using configuration:", this.cfg);


This will give us a better view on user's config in bug reports:

I'm not a fan of it. It's useful, but it gets pretty annoying if every extension does that. I mentioned it before, we could have a "report issue" command that dumps that stuff among other things.

Hmm, I wish we setup some logging-to-file infrastructure. There is a dedicated ExtensionContext.logPath property for this. But I think @matklad would like to keep console logging at max, though not sure...

Let's do this (in a separate PR):

define a function log somewhere, which just forwards to console.log

use log everywhere

add some config to opt-in into logging

I do think this is useful to print lots of stuff. But printing should be abstracted away, so that it's easier to redirect, silence, elevate to gui popups, etc. Ideally, we should use a built in logger, but there isn't one afaik, so we need to do our own.

Veetaha · 2020-02-13T21:00:22Z

editors/code/src/config.ts

-            return path.replace('~', os.homedir());
+    private static replaceTildeWithHomeDir(path: string) {
+        if (path.startsWith("~/")) {
+            return os.homedir() + path.slice("~".length);


@matklad, I corrected a bit your implementation, to avoid possible caveats of string.replace(). The second argument (which is os.homedir() in this case) is actually a pattern (sorry, template) where $', $$, $1, $2... tokens have special meaning, so I better not rely on user's homedir name, this may blow...

It also works better with paths that contain ~.

@lnicola, sorry, not sure what you meant. If you think that path.replace("~", blah) will replace all occurences of "~", be informed that it replaces only the first occurence:

I know, this is counterintuitive, I even heard about string.replaceAll() proposal

The pattern can be a string or a RegExp, and the replacement can be a string or a function to be called for each match. If pattern is a string, only the first occurrence will be replaced.

Oh... :-(

lnicola · 2020-02-13T21:13:54Z

editors/code/package.json

                "rust-analyzer.excludeGlobs": {
                    "type": "array",
+                    "items": {
+                        "type": "string"


lnicola · 2020-02-13T21:15:44Z

editors/code/src/config.ts

+
+    private refreshConfig() {
+        this.cfg = vscode.workspace.getConfiguration(Config.rootSection);
+        console.log("Using configuration:", this.cfg);


I'm not a fan of it. It's useful, but it gets pretty annoying if every extension does that. I mentioned it before, we could have a "report issue" command that dumps that stuff among other things.

matklad · 2020-02-14T11:33:36Z

editors/code/src/config.ts

-        }
-        this.prevCargoFeatures = { ...this.cargoFeatures };
+    // FIXME: add codegen for primitive configurations
+    highlightingOn()        { return this.cfg.get("highlightingOn") as boolean; }


Suggested change

highlightingOn() { return this.cfg.get("highlightingOn") as boolean; }

get highlightingOn() { return this.cfg.get("highlightingOn") as boolean; }

Seems like there's no reason not to use getters here?

I am not a fan of getters sugar... but I won't stand against.

@matklad do we make everything getters or only simple this.cfg.get("prop") ones?

I'd vote for everything

matklad

A very nice cleanup, thank you @Veetaha !

bors r+

matklad · 2020-02-14T21:08:15Z

editors/code/src/client.ts

@@ -1,39 +1,41 @@
 import * as lc from 'vscode-languageclient';
+import * as vscode from 'vscode';


@matklad

3131: vscode: simplified config and to removed one source of truth of default values r=matklad a=Veetaha Though not intended initially, the implementation of config design is alike [dart's one](https://github.com/Dart-Code/Dart-Code/blob/master/src/extension/config.ts) as pointed by @matklad in PM. Co-authored-by: Veetaha <gerzoh1@gmail.com>

bors · 2020-02-14T21:16:05Z

Build succeeded

Rust (macos-latest)
Rust (ubuntu-latest)
Rust (windows-latest)
TypeScript

Veetaha added 2 commits February 13, 2020 22:47

vscode: added more type safety to package.json config

9b47124

vscode: redesigned config with simplicity and Dart extension config i…

7ad15c3

…mplementation in mind

Veetaha requested review from kjeremy, lnicola and matklad and removed request for matklad February 13, 2020 20:51

Veetaha commented Feb 13, 2020

View reviewed changes

vscode: reordered config constructor before methods

fd37151

lnicola approved these changes Feb 13, 2020

View reviewed changes

matklad reviewed Feb 14, 2020

View reviewed changes

Veetaha added 2 commits February 14, 2020 23:04

vscode: moved to getters as per matklad

4fb4277

make onConfigChange handler private

20fabaf

Veetaha requested a review from matklad February 14, 2020 21:08

matklad reviewed Feb 14, 2020

View reviewed changes

bors bot merged commit 20fabaf into rust-lang:master Feb 14, 2020

This was referenced Feb 20, 2020

Improve RA version display #3247

Merged

Add logging infrastructure to TypeScript VSCode extension #3250

Closed

Veetaha deleted the feature/vscode-redesign-config branch February 24, 2020 20:11

Veetaha mentioned this pull request Mar 18, 2020

Alternative aproach to plugin auto update #3629

Merged

	highlightingOn() { return this.cfg.get("highlightingOn") as boolean; }
	get highlightingOn() { return this.cfg.get("highlightingOn") as boolean; }

		@@ -1,39 +1,41 @@
		import * as lc from 'vscode-languageclient';
		import * as vscode from 'vscode';

vscode: simplified config and to removed one source of truth of default values #3131

vscode: simplified config and to removed one source of truth of default values #3131

Uh oh!

Conversation

Veetaha commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Veetaha Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Veetaha Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matklad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors bot commented Feb 14, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Veetaha commented Feb 13, 2020 •

edited

Loading

Veetaha commented Feb 13, 2020 •

edited

Loading

Veetaha Feb 13, 2020 •

edited

Loading

Veetaha Feb 13, 2020 •

edited

Loading