tools icon indicating copy to clipboard operation
tools copied to clipboard

📎 Add `files` config option for `ignore`

Open sebmck opened this issue 2 years ago • 6 comments

Description

I found myself wanting to add the same directories/patterns to linter.ignore and formatter.ignore. It would be nice so that instead of:

{
  "formatter": {
    "ignore": ["vendor", "backend/build", "node_modules", "frontend", "infra/production-build/var", "infra/production-build/usr"]
  },
  "linter": {
    "ignore": ["vendor", "backend/build", "node_modules", "frontend", "infra/production-build/var", "infra/production-build/usr"]
  }
}

I could do:

{
  "files": {
    "ignore": ["vendor", "backend/build", "node_modules", "frontend", "infra/production-build/var", "infra/production-build/usr"]
  }
}

sebmck avatar Oct 04 '22 19:10 sebmck

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

github-actions[bot] avatar Oct 19 '22 12:10 github-actions[bot]

I take this issue and created the draft PR! Currently, I'm thinking about How treat "ignores" options in files config and linter config(or formatter config)

{
  "files": {
    "ignore": ["test1.js"]
  },
  "linter": {
    "ignore": ["test2.js"]
  }
}

Above the case, can I ignore test1.js and test2.js or only test2.js when linting? Please let me know if you have any advice.

nissy-dev avatar Nov 06 '22 12:11 nissy-dev

Above the case, can I ignore test1.js and test2.js or only test2.js when linting?

As a user, I would expect file1.js and file2.js to be ignored by the linter. file1.js should be ignored by all Rome analysis (except maybe invalid import detection...? unsure).

strager avatar Nov 07 '22 09:11 strager

I agree the ignore lists should be additive, meaning the linter should ignore paths that match any pattern in both files.ignore and linter.ignore. One thing we should probably support though is negative patterns (prefixed with ! like !**/lint_these/*.js) to let the user specify files that should be ignore by everything but the linter (or any other specific tool). Finally, I should note that the actual definition of what ignore does is tool-specific, for instance in the linter it may mean that no diagnostic or code action is emitted for this file but doesn't necessarily mean it's entirely exempt from analysis (similarly to how skipLibCheck in TypeScript means the declaration files do not emit any error, but the types they define are still used for type checking)

leops avatar Nov 07 '22 10:11 leops

I agree the ignore lists should be additive, meaning the linter should ignore paths that match any pattern in both files.ignore and linter.ignore.

What's your recommendation on the order in which the lists are "added". Because the order becomes important if you have negative patterns.

@leops is it correct that Rome ignores some files by default? If so, should files.ignore override the default ignore list?

@nissy-dev : Don't feel pressured to implement all the ideas in a single PR (or leave some of the functionalities to others).

MichaReiser avatar Nov 07 '22 11:11 MichaReiser

What's your recommendation on the order in which the lists are "added". Because the order becomes important if you have negative patterns.

Logically we should be concatenating the two lists with the global ignores coming first, then the tool-specific ignores

is it correct that Rome ignores some files by default? If so, should files.ignore override the default ignore list?

Yes rome_fs has a hard-coded list of directory names to ignore: https://github.com/rome/tools/blob/b6af8f0222fb30b28e37ef2fe8eb2e584583a95d/crates/rome_fs/src/fs/os.rs#L139

Whether the global ignore list should override these depends on whether we support negative patterns I guess. If we do then users could de-ignore some of these by adding !**/node_modules/** to the list for instance, but since we may not support this yet in the initial implementation we could have the global ignore list completely replace these defaults instead.

leops avatar Nov 07 '22 16:11 leops

Closing, as it's been implemented in https://github.com/rome/tools/pull/3564

ematipico avatar Dec 12 '22 16:12 ematipico