Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

📎 Add files config option for ignore #3328

Closed
sebmck opened this issue Oct 4, 2022 · 7 comments
Closed

📎 Add files config option for ignore #3328

sebmck opened this issue Oct 4, 2022 · 7 comments
Assignees
Labels
A-Project Area: project configuration and loading S-Planned Status: planned by the team, but not in the short term task A task, an action that needs to be performed

Comments

@sebmck
Copy link
Contributor

sebmck commented Oct 4, 2022

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 sebmck added the task A task, an action that needs to be performed label Oct 4, 2022
@ematipico ematipico added the A-Project Area: project configuration and loading label Oct 5, 2022
@ematipico ematipico added this to the 10.0.0 milestone Oct 5, 2022
@github-actions
Copy link

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

@ematipico ematipico added S-Planned Status: planned by the team, but not in the short term and removed S-Stale labels Oct 19, 2022
@ematipico ematipico modified the milestones: 10.0.0, 10.1.0 Nov 2, 2022
@nissy-dev
Copy link
Contributor

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.

@strager
Copy link
Contributor

strager commented Nov 7, 2022

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).

@leops
Copy link
Contributor

leops commented Nov 7, 2022

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)

@MichaReiser
Copy link
Contributor

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).

@leops
Copy link
Contributor

leops commented Nov 7, 2022

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:

const DEFAULT_IGNORE: &[&str; 5] = &[".git", ".svn", ".hg", ".yarn", "node_modules"];

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.

@ematipico
Copy link
Contributor

Closing, as it's been implemented in #3564

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Project Area: project configuration and loading S-Planned Status: planned by the team, but not in the short term task A task, an action that needs to be performed
Projects
Status: Done
Development

No branches or pull requests

6 participants