feat: auto-discover config file #13313

ry · 2022-01-08T18:06:05Z

No description provided.

bartlomieju · 2022-01-08T22:19:51Z

cli/config_file.rs

+ for f in flags.config_path_args() {
+ if let Some(cf) = discover_inner(&f, &mut checked)? {
+ return Ok(Some(cf));
+ }
+ }


This is a bit unexpected - IIRC the algorithm we agreed upon was supposed to find a config file for each of the passed arguments, not a config file that was found first. Could you clarify this @ry?

I've been thinking about this more just now and I think this goes beyond what we've discussed in the past. I'm wondering if when running deno fmt or deno lint should we have deno.json files in sub directories be resepected when there's a deno.json in an ancestor directory.

For example, say you have:

sub_dir - deno.json - file.ts deno.json

Should running deno fmt have the same behaviour on file.ts as running deno fmt sub_dir/file.ts?

Should running deno fmt sub_dir/file.ts have the same behaviour as running (cd sub_dir && deno fmt file.ts)?

I can think of a few options:

Option 1: Resolve based on closest deno.json from file path

This option would mean for deno fmt and deno lint along with deno fmt <file-path>, it would always resolve a config file based on the closest deno.json. So includes/excludes in parent deno.jsons would be ignored.

Fmt/lint option resolution: Each config file resolves from the default options

Includes/excludes: Each config file resolves from the default options

Run options: Resolved from closest ancestor deno.json (Same as Option 3)

Personally, I think this is the best option and is the least complex for users to understand. It answers yes to both the questions I asked above.

Option 2: Resolve using resolution from all deno.json files in ancestor directories from file path

This would require resolving all deno.json files in all ancestor directories, then use that information to figure out the fmt/lint options and includes/excludes for a folder based on the combined result of all config files.

Fmt/lint option resolution: Each config file resolves from the ancestor directory's options

Includes/excludes: Each config file resolves from the ancestor directory's includes/excludes and overrides it.

Run options: Requires resolution of each ancestor config and then a resolution to the final options by combining configs.

This would be complex to implement. It answers yes to both the questions I asked above.

Option 3: Use config file resolved for root of each disjointed tree

By "disjointed tree" I mean, directory sub trees that share no common directories from the root with other directory trees. Maybe there is a better word?

This would resolve the config file for each disjointed tree. So deno fmt would resolve based on the cwd and use that, but deno fmt ./child/mod.ts ../sibling/mod.ts ../sibling/other.ts would resolve the config file for ./child and ../sibling to use for their respective files.

Fmt/lint option resolution: Each config file resolves from the root of each disjointed tree

Includes/excludes: Each config file resolves from the root of each disjointed tree

Run options: Resolved from closest ancestor deno.json (same as Option 1)

I think this is the solution we agreed upon before? It answers no to both of the questions I asked above.

Option 1 and Option 3 are the same except applying to multiple arguments. I agree with this approach.

Or we can just find the deno.json based on the first argument and if other files have a different config, erroniously ignore it for the time being. These are corner cases that we should have tests for anyway - we can move to option 1/3 in the future.

I believe Option 3 was exactly what we discussed previously.

kitsonk · 2022-01-10T00:59:11Z

Is this shippable without addressing the language server? Feels like it should be included in this PR.

ry · 2022-01-10T01:06:48Z

Is this shippable without addressing the language server? Feels like it should be included in this PR.

What needs to be addressed in the language server?

kitsonk · 2022-01-10T03:23:50Z

Currently the lsp requires an explicit option of "config" to be set in the client configuration for a configuration file to apply:

deno/cli/lsp/config.rs

Lines 157 to 159 in c487b7e

 /// An option that points to a path string of the config file to apply to 

 /// code within the workspace. 

 pub config: Option<String>,

And when there is something impacted in the environment, like when the client configuration or the configuration file iteself changes, then the language server processes an update:

deno/cli/lsp/language_server.rs

Line 524 in c487b7e

fn update_config_file(&mut self) -> Result<(), AnyError> {

If we have automatic behaviour to identify the file when deno run, then it only seem logical that the behaviour would be similar by default for the language server, not requiring explicit configuration, which mirrors the command line today, and I would assume that delivery of that functionality would be part of this feature, since a fair amount of people utilise the language server.

Generally this should be fairly straight forward with just a "workspace" and looking in the "workspace root", but when you have "workspace folders" (multi-root workspaces), I suspect it could get a bit convoluted.

Side note, I just realised we are only watching JSON files in the workspace (and not JSONC):

deno/cli/lsp/language_server.rs

Lines 722 to 728 in c487b7e

 let watch_registration_options = 

 DidChangeWatchedFilesRegistrationOptions { 

 watchers: vec![FileSystemWatcher { 

 glob_pattern: "**/*.json".to_string(), 

 kind: Some(WatchKind::Change), 

 }], 

 };

And not watching for changes in .jsonc files, but if a configuration file is identified, it can be registered for changes, I believe.

Another discussion at one point, which could be delivered later, is that if we automatically detect a deno.json/deno.jsonc it would imply that the project/workspace was implicitly Deno "enabled" not requiring explicit enablement as it does today.

ry · 2022-01-10T03:55:53Z

Could we just rip out the LSP config option? Seems like if we have auto-discover then maybe this option isn't necessary

kitsonk · 2022-01-10T04:00:06Z

Will the CLI still allow --config to be explicitly specified? If so, then you need a similar capability in the configuration of the language server.

ry · 2022-01-15T17:07:51Z

@kitsonk I put up a little test case but it doesn't seem to be working. I'm seeing this error message from the LSP

Error retrieving settings for specifier (file:https:///var/folders/9v/kys6gqns6kl8nksyn4l1f9v40000gn/T/.tmpxnaESN/ignored.ts): Parse error: invalid type: map, expected a sequence

Any advice would be appreciated

kitsonk · 2022-01-15T20:34:57Z

Ah, OK, I will have to trace it through, but what is happening, versus the other tests like this, is that it is triggering the workspace folder logic which request from the client the specific configuration for a file, and for some reason it is causing this test to fail.

bartlomieju · 2022-01-15T21:59:48Z

Tried this PR on deno_std, works nicely :)

kitsonk · 2022-01-16T23:48:40Z

cli/tests/integration/lsp_tests.rs

- )
- .unwrap();
-
+ let workspace_root = temp_dir.path().canonicalize().unwrap();


This was the fix, in that auto-discovery automatically canonicalizes the discovered path.

An open question though is how do clients behave? Do they canonicalize files? There isn't an easy way to determine. We may need to fix exclude and include properties for things like format and lint to canoncilize files... 😕

kitsonk · 2022-01-17T21:05:47Z

Looks like something is hanging with lsp_diagnostics_refresh_dependents...

kitsonk · 2022-01-17T21:55:52Z

Looks like something is hanging with lsp_diagnostics_refresh_dependents...

Found the problem, will push a fix after I check the rest of the tests...

kitsonk

LGTM

bartlomieju

LGTM too.

There are some unrelated changes in this PR, but I'm not gonna nitpick.

dsherret

LGTM too!

lucacasonato

Tried it out locally, and it is working great. LGTM!

jsejcksn · 2022-01-21T06:35:09Z

I don't see anything in the release notes about how to disable this feature. How can I prevent deno from walking the directory hierarchy and discovering configs?

ry added 7 commits January 4, 2022 10:15

wip

4a76234

wip kinda working

f1d9241

wip

795f2eb

test_extract_path_args

9dca2f8

use method

3bbfb8e

improvements

c020e69

malformed_config test

f5aecf8

ry requested review from bartlomieju and bnoordhuis as code owners January 8, 2022 18:06

ry added 2 commits January 8, 2022 13:09

lint

a0aa811

fix fmt_with_config tests, move is_supported_ext_fmt to cli/tools/fmt.rs

1b84c32

ry requested a review from dsherret as a code owner January 8, 2022 19:09

ry added 2 commits January 8, 2022 14:12

fix

c8e4511

Merge branch 'main' into auto-config

f9194c1

bartlomieju reviewed Jan 8, 2022

View reviewed changes

ry added 2 commits January 10, 2022 18:22

Merge branch 'main' into auto-config

67756b5

support lsp

514c349

ry requested a review from kitsonk as a code owner January 11, 2022 00:27

Soremwar mentioned this pull request Jan 13, 2022

auto discover config file #13359

Closed

ry added 2 commits January 15, 2022 11:13

Merge branch 'main' into auto-config

a91fbcc

wip test case for lsp

7619d58

fix lsp integration test and get rid of noisy error msg

bac55fa

kitsonk reviewed Jan 16, 2022

View reviewed changes

kitsonk and others added 4 commits January 17, 2022 12:29

lint

d621068

Merge branch 'main' into auto-config

28e4e71

Merge branch 'main' into auto-config

0362545

undo stderr change

259679a

kitsonk and others added 4 commits January 18, 2022 09:01

fix hanging lsp test

df74ff1

Merge branch 'main' into auto-config

38019a0

remove todo

f29db8a

fix

fe326df

kitsonk approved these changes Jan 17, 2022

View reviewed changes

bartlomieju approved these changes Jan 17, 2022

View reviewed changes

dsherret approved these changes Jan 17, 2022

View reviewed changes

lucacasonato approved these changes Jan 18, 2022

View reviewed changes

ry merged commit 39ea4ab into main Jan 18, 2022

bartlomieju mentioned this pull request Jan 18, 2022

Deprecate "_tools/" scripts denoland/deno_std#1830

Closed

kitsonk mentioned this pull request Jan 21, 2022

Disable auto-detection of config file #13452

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto-discover config file #13313

feat: auto-discover config file #13313

ry commented Jan 8, 2022

bartlomieju Jan 8, 2022

dsherret Jan 10, 2022 •

edited

Loading

ry Jan 10, 2022

bartlomieju Jan 10, 2022

kitsonk commented Jan 10, 2022

ry commented Jan 10, 2022

kitsonk commented Jan 10, 2022

ry commented Jan 10, 2022

kitsonk commented Jan 10, 2022

ry commented Jan 15, 2022 •

edited

Loading

kitsonk commented Jan 15, 2022

bartlomieju commented Jan 15, 2022

kitsonk Jan 16, 2022

kitsonk commented Jan 17, 2022

kitsonk commented Jan 17, 2022

kitsonk left a comment

bartlomieju left a comment

dsherret left a comment

lucacasonato left a comment

jsejcksn commented Jan 21, 2022 •

edited

Loading

feat: auto-discover config file #13313

feat: auto-discover config file #13313

Conversation

ry commented Jan 8, 2022

bartlomieju Jan 8, 2022

Choose a reason for hiding this comment

dsherret Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

ry Jan 10, 2022

Choose a reason for hiding this comment

bartlomieju Jan 10, 2022

Choose a reason for hiding this comment

kitsonk commented Jan 10, 2022

ry commented Jan 10, 2022

kitsonk commented Jan 10, 2022

ry commented Jan 10, 2022

kitsonk commented Jan 10, 2022

ry commented Jan 15, 2022 • edited Loading

kitsonk commented Jan 15, 2022

bartlomieju commented Jan 15, 2022

kitsonk Jan 16, 2022

Choose a reason for hiding this comment

kitsonk commented Jan 17, 2022

kitsonk commented Jan 17, 2022

kitsonk left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

dsherret left a comment

Choose a reason for hiding this comment

lucacasonato left a comment

Choose a reason for hiding this comment

jsejcksn commented Jan 21, 2022 • edited Loading

dsherret Jan 10, 2022 •

edited

Loading

ry commented Jan 15, 2022 •

edited

Loading

jsejcksn commented Jan 21, 2022 •

edited

Loading