Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
niedzielski opened this issue Jul 24, 2022 · 8 comments · Fixed by #17778
Labels
config Related to configuring Deno via deno.json/deno.jsoc suggestion suggestions for new features (yet to be agreed)

Comments

@niedzielski
Copy link

niedzielski commented Jul 24, 2022

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

@kitsonk kitsonk added suggestion suggestions for new features (yet to be agreed) config Related to configuring Deno via deno.json/deno.jsoc labels Jul 24, 2022
@haiderlikesrust
Copy link

Interesting I’ll look into the code later.

@scarf005
Copy link
Contributor

scarf005 commented Feb 14, 2023

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

#[derive(Clone, Debug, Default, Eq, PartialEq)]
pub struct FilesConfig {
pub include: Vec<PathBuf>,
pub exclude: Vec<PathBuf>,
}

"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

pub fn to_fmt_config(&self) -> Result<Option<FmtConfig>, AnyError> {
if let Some(config) = self.json.fmt.clone() {
let fmt_config: SerializedFmtConfig = serde_json::from_value(config)
.context("Failed to parse \"fmt\" configuration")?;
Ok(Some(fmt_config.into_resolved(&self.specifier)?))
} else {
Ok(None)
}
}

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.

@dsherret
Copy link
Member

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?

@Im-Beast
Copy link

what about tooling.files?

@scarf005
Copy link
Contributor

scarf005 commented Feb 15, 2023

some possible solutions:

only allow exclude array

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

use "tooling" scope

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

@selurvedu
Copy link

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

@dsherret
Copy link
Member

dsherret commented Oct 25, 2023

@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 #20813 once that is implemented.

@selurvedu
Copy link

selurvedu commented Oct 25, 2023

@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 {}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Related to configuring Deno via deno.json/deno.jsoc suggestion suggestions for new features (yet to be agreed)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants