search

atom / fuzzy-finder

Find and open files quickly
MIT License
276 stars 138 forks source link

Ignore VCS directories even when they are not in ignoreNames #399

Closed aviatesk closed 5 years ago

aviatesk commented 5 years ago

Hi, this is my very first PR for Atom's core system ! 😁

Rationale

If an user's core.ignoredNames is modified so that it doesn't include .git or .hg (they are included by default) but still core.excludeVcsIgnoredPaths is true, then the new fast mode using ripgrep will show files in those directories (like HEAD, COMMIT_EDITMSG and so on).

Because those files are usually regarded as VCS-ignored files and more over, the previous alternative mode does exclude those files when core.excludeVcsIgnoredPaths is true, I believe these files are better to be ignored in fast mode as well.

Please refer to https://github.com/atom/fuzzy-finder/issues/379#issuecomment-502635850 for more detail.

Description of the Change

Only 5 line changes: Add ripgrep arguments to exclude those files when core.excludeVcsIgnoredPaths is true.

Possible Drawbacks

To include those files, an user need to disable both core.ignoredNames and core.excludeVcsIgnoredPaths, and it might be confusing ?

rafeca commented 5 years ago

Wow, this was fast!! Thanks a lot for the PR! 😍

Don't worry about the failure on travis, it's not caused by this PR.

Can you also add a test, so this behaviour does not get broken in the future? You can add it just below this test:

https://github.com/atom/fuzzy-finder/blob/f8e0cb14aa5fa43e6d477f1656b80646a2ff935b/spec/fuzzy-finder-spec.js#L1659-L1672

You can use that test as an inspiration, and do something like atom.config.set('core.ignoredNames', []) at the beginning to be sore that the ignoredNames param does not have the .git folder in it.

To include those files, an user need to disable both core.ignoredNames and core.excludeVcsIgnoredPaths, and it might be confusing ?

I think it's such a rare case to want to search inside the .git folder that this should be fine. Alternatively once this PR lands we can remove the .git folder from the core.ignoredNames default value.

aviatesk commented 5 years ago

I'm welcome to write specs ! 😄 Not too much sure about writing specs, but there seem a lot of examples in fuzzy-finder, thus I may make it through. Thanks for your kind mentoring !

I think it's such a rare case to want to search inside the .git folder that this should be fine. Alternatively once this PR lands we can remove the .git folder from the core.ignoredNames default value.

Yeah, I totally agree with you and the alternative also sounds sane.

aviatesk commented 5 years ago

@rafeca I added the spec just now ! On my local machine, I confirmed this PR can pass but the current master branch fails the spec I've just added.

I've not created a spec for the case when core.excludeVcsIgnoredPaths is set to false, since I thought it would make specs more verbose. But if you want, I'm okay to add a spec for the case 😃

aviatesk commented 5 years ago

Thanks again for your help !

I'm really happy to be able to contribute to Atom, my favourite editor, and yeah, will make more in the future ! 😁

Looking forward to the next release !