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

update FindFiles2 to make options clearer #211494

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

andreamah
Copy link
Contributor

@andreamah andreamah commented Apr 26, 2024

Fixes #205692

Edits the FindFiles2 API to be clearer in the exclude/ignore behavior.

@andreamah andreamah self-assigned this Apr 26, 2024
@VSCodeTriageBot VSCodeTriageBot added this to the April 2024 milestone Apr 26, 2024
bhavyaus
bhavyaus previously approved these changes Apr 26, 2024
export enum SearchIgnoreOptions {
none = 1,
localOnly = 2,
localAndFollowSettings = 3,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which settings? does followSettings means to follow what's set in excludeSettingOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means to follow the settings for global and parent ignore files-following. So using those settings to dictate whether to use them.

Copy link
Contributor

@rzhao271 rzhao271 May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the option be changed to followIgnoreSettings or a note be added to the docstring then? Otherwise, I think it's really easy to mix up these followSettings keys with the exclude enum options and/or exclude settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that option 2 in this comment makes anything more clear? #205692 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this PR has the enum approach, I've been brainstorming better ways to go about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of useDefaultExcludes and useDefaultSearchExcludes is confusing
4 participants