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

Add .gitignore support #2492

Closed
Tracked by #2316
NicholasLYang opened this issue Apr 25, 2022 · 18 comments · Fixed by #4375
Closed
Tracked by #2316

Add .gitignore support #2492

NicholasLYang opened this issue Apr 25, 2022 · 18 comments · Fixed by #4375
Labels
A-CLI Area: CLI enhancement New feature request or improvement to existing functionality S-Planned Status: planned by the team, but not in the short term S-Wishlist Possible interesting features not on the current roadmap task A task, an action that needs to be performed

Comments

@NicholasLYang
Copy link
Contributor

Currently we have hard coded folders that we ignore. We should change that to follow the .gitignore

This was referenced Apr 25, 2022
@ematipico
Copy link
Contributor

What are the requirements of .gitignore? Is it necessary?

My concern is that we should not ignore files that are ignored by the VCS. Mainly because Rome is not a VCS and there's value in processing files that are ignored.

It can provide value for operations that involve VCS, so we should carefully understand the when and how.

@yassere yassere added this to Rome 2022 May 8, 2022
@NicholasLYang
Copy link
Contributor Author

IMO we should revisit this along with #2493. It's an extremely common use case and has prior art in ripgrep and fd.

@ematipico
Copy link
Contributor

Should we add support to of .gitignore file only when we are inside a project that has our config file? (rome.json)

We don't have support of a configuration file yet, but once we do, we could add support for .gitignore too.

@NicholasLYang
Copy link
Contributor Author

Would that be the ideal setup? I'm not a huge fan of dprint requiring a config file to use the formatter. While a rome.json would not be mandatory, I anticipate that the majority of people will want to run the formatter on only files that are checked into git

@ematipico
Copy link
Contributor

ematipico commented May 24, 2022

What if:

  • you have rome.json, then the ignored files will go inside the relative ignore section (as said before, there's value in processing files that are ignored inside a VCS);
  • you don't have rome.json, then we use .gitignore, with all the ups and downs that come with it;

@NicholasLYang
Copy link
Contributor Author

That could work, although we'd have to figure out a way to make that clear to users. It might be very confusing to have Rome obey the .gitignore, then when a rome.json file is added, suddenly it ignores the .gitignore.

@ematipico
Copy link
Contributor

IMO we should revisit this along with #2493. It's an extremely common use case and has prior art in ripgrep and fd.

I've been thinking more about this, and I am more convinced that we should not supported this out of the box (maybe behind flag). The examples you mentioned are different from us (they don't write files, for example). If you have examples of CLIs similar to Rome (formatter, bundler, test framework, linter, etc.), that would build your case.

Still, I think it should an opt in feature, if we really want to support it.

@ematipico
Copy link
Contributor

ematipico commented Aug 3, 2022

Closing, ignored files will be part of the configuration file

@ematipico ematipico moved this to Done in Rome 2022 Aug 3, 2022
@IanVS
Copy link
Contributor

IanVS commented Nov 9, 2022

Hi, just wanted to voice my personal experience. I just downloaded rome to try it out, and was surprised when it tried to format files that are in my .gitignore. I find it mildly annoying that I need to duplicate my .gitignore in my rome config. Feel free to ignore me, I'm just providing a bit of unsolicited feedback. :)

@sebmck
Copy link
Contributor

sebmck commented Nov 9, 2022

@IanVS Yeah this is good feedback. I think we should likely consider doing this.

@sebmck sebmck reopened this Nov 9, 2022
@mbrevda
Copy link

mbrevda commented Nov 15, 2022

does #2493 help here?

@ematipico
Copy link
Contributor

Hi, just wanted to voice my personal experience. I just downloaded rome to try it out, and was surprised when it tried to format files that are in my .gitignore. I find it mildly annoying that I need to duplicate my .gitignore in my rome config. Feel free to ignore me, I'm just providing a bit of unsolicited feedback. :)

Do you know if other tools - e.g. prettier, eslint, esbuild, etc. - ignore files inside the .gitignore folder automatically?

@mbrevda
Copy link

mbrevda commented Nov 15, 2022

Prettier allows passing an --ignore-path option which accepts any gitignore-compatable file. So while it doesn't read .gitignore by default it is simple to instruct it to

@Mathspy
Copy link

Mathspy commented Nov 15, 2022

  • prettier: doesn't use .gitignore automatically but the syntax for its ignore files is exactly the same as .gitignore syntax and it has has a CLI argument --ignore-path to use a specific file; prettier --ignore-path .gitignore
  • eslint: doesn't use .gitignore automatically but the syntax for its ignore files is exactly the same as .gitignore syntax and it has a CLI argument --ignore-pattern to use it; eslint --ignore-pattern .gitignore
  • esbuild: what esbuild does is slightly different from other tools which implies that if a file is imported it should be used regardless of existence of .gitignore or not. A lot of the time gitignored folders are either node_modules or are generated JS files that would bloat the repo but are essential to include in a bundle.
  • tokei: will automatically respect gitignore
  • ripgrep: will automatically respect gitignore

A lot of the modern Rust tools tend to respect .gitignore automatically, if it makes sense for what they are doing. Which I believe is a good default for both formatting and linting. Generated JS/CSS/JSON files don't make sense to format or lint the same way they don't make sense to count the lines of or search through. It also saves a lot of unnecessary recursive searching and remove the currently hardcoded node_modules special case.

@MichaReiser MichaReiser added enhancement New feature request or improvement to existing functionality task A task, an action that needs to be performed A-CLI Area: CLI labels Nov 16, 2022
@ematipico
Copy link
Contributor

Yeah, I think it makes sense to try to cover the majority of the cases and meet the users halfway.

Still, I think we should provide an option to opt out this feature - if it becomes the default. I know it seems weird, but as a web developer - and especially a tool developer - many times I wanted to run locally the very tool on some files while keeping them ignored in git. Surely these use cases are way more less then the average, so opt-out the .gitignore might work.

@github-actions
Copy link

👋 @rome/staff please triage this issue by adding one of the following labels: S-Bug: confirmed, S-Planned , S-Wishlist or umbrella

@MichaReiser MichaReiser added S-Wishlist Possible interesting features not on the current roadmap S-Stale and removed S-Stale labels Nov 30, 2022
@ematipico ematipico removed the S-Stale label Nov 30, 2022
@ematipico ematipico added the S-Planned Status: planned by the team, but not in the short term label Nov 30, 2022
@Th3S4mur41
Copy link

There is currently a similar discussion going on in prettier...
This shows that there is a need at least for an option to reuse .gitignore in the formatter, even though not as the only config for ignoring files.

@ematipico
Copy link
Contributor

ematipico commented Mar 29, 2023

Yeah, it makes sense to add support for it. My only concern was a way to opt out of this feature. Other than that, let us make a PR to support it!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI enhancement New feature request or improvement to existing functionality S-Planned Status: planned by the team, but not in the short term S-Wishlist Possible interesting features not on the current roadmap task A task, an action that needs to be performed
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants