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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jun 18, 2019

Conversation

Projects
None yet
2 participants
@aviatesk
Copy link
Contributor

commented Jun 18, 2019

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 #379 (comment) 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

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

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:

describe('when the .gitignore matches parts of the path to the root folder', () => {
beforeEach(() => {
const ignoreFile = path.join(projectPath, '.gitignore')
fs.writeFileSync(ignoreFile, path.basename(projectPath))
})
it('only applies the .gitignore patterns to relative paths within the root folder', async () => {
await projectView.toggle()
await waitForPathsToDisplay(projectView)
expect(Array.from(projectView.element.querySelectorAll('li')).find(a => a.textContent.includes('file.txt'))).toBeDefined()
})
})

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 aviatesk changed the title Ignore VSC directories even when they are not in ignoreNames Ignore VCS directories even when they are not in ignoreNames Jun 18, 2019

@aviatesk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

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 added some commits Jun 18, 2019

Add a spec that covers excluding VCS dot directory
The spec checks .git and .hg directories are ignored when 
`core.ignoredNames` has no glob pattern for them

@aviatesk aviatesk force-pushed the aviatesk:default-vcs-ignore branch from 2e175f0 to 82a39c4 Jun 18, 2019

@aviatesk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

@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 馃槂

@rafeca

rafeca approved these changes Jun 18, 2019

Copy link
Contributor

left a comment

This is awesome! 鉂わ笍

Thanks a lot for this PR, and congratulations for your first contribution to Atom! I hope that many more will come afterwards 馃槂

I'm going to merge it and publish a new version of the fuzzy finder package. This fix will probably be available on v1.39 (since I'll cherry-pick this along some other stuff from fuzzy finder to the current beta).

@rafeca rafeca merged commit 31ad8f1 into atom:master Jun 18, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@aviatesk

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

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 !

@aviatesk aviatesk deleted the aviatesk:default-vcs-ignore branch Jun 18, 2019

aviatesk added a commit to aviatesk/avi-atom that referenced this pull request Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.