deno icon indicating copy to clipboard operation
deno copied to clipboard

[feature request] Add top-level `files` deno.json config to replace `fmt.files` + `lint.files` + `test.files`

Open niedzielski opened this issue 2 years ago • 1 comments

I have a JavaScript monorepo wherein some sub-projects do not comply to Deno. To fully ignore them, I have to add three files.exclude configs to my deno.json:

{
  "fmt": {
    "files": { "exclude": ["modules/tools"] },
    // More options.
  },
  "lint": {
    "files": { "exclude": ["modules/tools"] },
    // More options.
  },
  "test": {
    "files": { "exclude": ["modules/tools"] },
    // More options.
  },
  // More options.
}

I'd like a single top-level files config that had the same effect:

{
  "files": { "exclude": ["modules/tools"] },
  "fmt": {
    // More options.
  },
  "lint": {
    // More options.
  },
  "test": {
    // More options.
  },
  // More options.
}

This DRYs up the config at the expense of increasing the complexity. It feels more natural to me than updating three places to effectively fully ignore a directory.

References

niedzielski avatar Jul 24 '22 16:07 niedzielski

Interesting I’ll look into the code later.

haiderlikesrust avatar Jul 25 '22 02:07 haiderlikesrust

Summary

to implement this feature, we need to handle cases like

{
  "files": { "exclude": ["npm/"] }
}

The four cases

there's 4 possible combination for global "files" and local config:

  1. no global "files" + config w/wo local "files"
  2. global "files" + config w/wo local "files"
  3. no global "files" + no config
  4. global "files" + no config

while case 1, 2 and 3 is unambigous, i'm unsure how to handle case 4.

no global "files" + config w/wo local "files"

same as before.

global "files" + config w/wo local "files"

{
  "files": { "exclude": ["npm/"] },
  "fmt": { "files": { "exclude": ["cov/"] } },
  "lint": {}
}

would be resolved to

{
  "fmt": { "files": { "exclude": ["cov/", "npm/"] } },
  "lint": { "exclude": ["npm/"] }
}

no global "files" + no config

same as before.

global "files" + no config

{
  "files": { "exclude": ["npm/"] }
}

how should this case be handled?

Possible interpretion

Implicit Resolving

Include "files" regardless local config. above case will be resolved as

{
  "files": { "exclude": ["npm/"] },
  "fmt": { "files": { "exclude": [ "npm/"] } }
}

Explicit Resolving

Only Include "files" if local config exists. above case will be resolved as

{}

Rust implmentation

implementation will change by how we decide to resolve files whether implicitly or explicitly.

Global "files" config

https://github.com/denoland/deno/blob/2502a37d41e8a7f279af74d7dacc009ee1010f67/cli/args/config_file.rs#L316-L320 "files" is represented as FilesConfig.

 pub fn to_files_config(&self) -> Result<Option<FilesConfig>, AnyError>

there could be a to_files_config impl that returns optional filesconfig result.

"Local" Configs

https://github.com/denoland/deno/blob/2502a37d41e8a7f279af74d7dacc009ee1010f67/cli/args/config_file.rs#L688-L696

FmtConfig, LintConfig, TestConfig is generated from ConfigFile. via to_{}_config impl. it returns Result<Option<{ConfigName}>, AnyError>. all three of them contains FilesConfig as their field.

Resolving config type

use match boilerplate

we could create a helper function that would do something like

implicit resolving: Option<FilesConfig> + Option<FmtConfig> -> FmtConfig

explicit resolving: Option<FilesConfig> + Option<FmtConfig> -> Option<FmtConfig>

the possible drawback is that we'd need to rewrite same code for each fmt, lint and test, which is a bit bulky:

let filesConfigOption = Option<FilesConfig>;
let fmtConfigOption = Option<FmtConfig>;
match (filesConfigOption, fmtConfigOption) {
    (Some(filesConfig), Some(fmtConfig)) => // ...
    (None, Some(fmtConfig)) => // ...
    (Some(filesConfig), None) => // ...
    (None, None) => // ...
}

however it's least intrusive (no need to change struct/return type).

use generic local config

struct WithFilesConfig<T> {
    data: T, // FmtConfig, LintConfig, TestConfig
    files: FilesConfig,
}

impl WithFilesConfig<T> {
    fn extend_files(self, files: FilesConfig) -> WithFilesConfig<T> {
        WithFilesConfig {
            files: self.files + files,
            ..self
        }
    }
}

it would remove the need to write duplicate code, but it could potentially break existing code and would need large refactoring.

in short, using match boilerplate would be preferred.

Others

since i'm new to rust, please point out if anything is incorrect. thanks in advance.

scarf005 avatar Feb 14 '23 06:02 scarf005

A problem about this, is that it creates some confusion around what this applies to (ex. it creates the question: do the cache, vendor, compile, doc subcommands grab from the "files" array?) and I wonder if this will cause issues in the future with future subcommands. It seems like the predominant usecase is to "exclude" so maybe this should be exclude only?

dsherret avatar Feb 14 '23 15:02 dsherret

what about tooling.files?

Im-Beast avatar Feb 14 '23 15:02 Im-Beast

some possible solutions:

only allow exclude array

{
  "exclude": ["npm/"]
  "fmt": {}
}

use "tooling" scope

{
  "tooling": {
    "filter": ["fmt", "lint", "bench"]
    "files": { "exclude": ["npm/"] }
  },
  "fmt": {}
}

scarf005 avatar Feb 15 '23 01:02 scarf005

#17778 doesn't seem to work with deno check.

selurvedu avatar Oct 24 '23 23:10 selurvedu

@selurvedu the excludes property added in #17778 was not for deno check. If say a.ts is in the excludes and you do deno check a.ts, then it will still type check a.ts. Similarly, if mod.ts imports a.ts it will still type check a.ts because a.ts is part of that module graph.

That said, the "exclude" property will be taken into account for https://github.com/denoland/deno/issues/20813 once that is implemented.

dsherret avatar Oct 25 '23 00:10 dsherret

@dsherret thanks for clarifying. I guess I could use this for now:

--- deno.json
+++ deno.json
@@ -1,7 +1,7 @@
 {
   "lock": true,
   "tasks": {
-    "check": "deno fmt --check && deno lint && deno check **/*.ts && deno check **/*.tsx",
+    "check": "deno fmt --check && deno lint && find . -type d -name _ignored -prune -o -type f -iname '*.ts' -o -iname '*.tsx' -print | xargs deno check",
     "start": "deno run -A --watch=routes/,components/,islands/ dev.ts",
     "build": "deno run -A dev.ts build && deno cache main.ts",
     "update": "deno run -A -r https://fresh.deno.dev/update .",

This was supposed to include find . -type d -name _ignored -prune -o -type f -iname '*.ts' -o -iname '*.tsx' -exec deno check {} +, but I don't know how to escase {}.

selurvedu avatar Oct 25 '23 00:10 selurvedu