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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix excluding VCS ignored paths based on the flag in core settings when ripgrep is enabled #395

Merged
merged 2 commits into from May 30, 2019

Conversation

Projects
None yet
3 participants
@nikazooz
Copy link
Contributor

commented May 29, 2019

Description of the Change

This fixes a bug when experimental fast mode that uses ripgrep is enabled. Since excluding VCS ignored paths is the default behavior of ripgrep, option to not ignore them was not being passed when needed.

Alternate Designs

N/A

Benefits

Excluding VCS ignored paths from fuzzy search will work a bit more consistently when using ripgrep.

Possible Drawbacks

N/A

Applicable Issues

N/A

@50Wliu

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Thanks @nikazooz! Can you please add a test for this to make sure it doesn't regress in the future?

nikazooz added some commits May 29, 2019

Fix excluding VCS ignored paths based on the flag in core settings
The `ignoreVcsIgnores` property was not set in the PathLoader constructor and
when it was used later it didn't hold any value so the option to not ignore
the VCS ignored paths could never be passed to ripgrep. As ignoring is the
default behavior, adding the option to NOT ignore needs to be done when
`core.excludeVcsIgnoredPaths` is `false`.

@nikazooz nikazooz force-pushed the nikazooz:fix-ripgrep-ignores branch from fb79f35 to 6d078e7 May 29, 2019

@nikazooz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

@50Wliu I don't have much experience with writing tests for atom packages, but this should cover the case this PR solves. I'm open to suggestions for improvement if you have any 🙂

@50Wliu

50Wliu approved these changes May 29, 2019

Copy link
Member

left a comment

This looks good to me - /cc @rafeca for some more 👀

@rafeca

rafeca approved these changes May 29, 2019

Copy link
Contributor

left a comment

Thanks for the fix @nikazooz, looks great 😍! And apologies for the issue 😅

@nikazooz

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Glad I was able to help 🙂
Thank you all for your work on this editor!

@50Wliu

This comment has been minimized.

Copy link
Member

commented May 30, 2019

Travis is having rate limit issues, so I think this is fine to merge.

@50Wliu 50Wliu merged commit 177cb6e into atom:master May 30, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.