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

Exclude **/node_modules/** from .java files search #3348

Merged

Conversation

gluxon
Copy link
Contributor

@gluxon gluxon commented Oct 20, 2023

This is a strange PR and I would have no qualms with it being rejected.

Similar to microsoft/vscode-java-debug#1234, I'm hoping to ignore node_modules to prevent large CPU usage from this extension when my teammates open a Node.js project. There's details in the linked pull request, but happy to elaborate if there's questions. Thanks!

Specifically, we're seeing this process run for long periods of time. It's searching on /{src, test}/**/*.java.

❯ ps x | grep ripgrep
93718   ??  R      0:16.30 /Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar.unpacked/@vscode/ripgrep/bin/rg --files --hidden --case-sensitive --no-require-git -g /{src, test}/**/*.java -g !**/.git -g !**/.svn -g !**/.hg -g !**/CVS -g !**/.DS_Store -g !**/Thumbs.db --no-ignore --no-config

@jdneo
Copy link
Collaborator

jdneo commented Oct 23, 2023

This utility method might help (there is a type of the method name, should be glob instead of blob):

vscode-java/src/utils.ts

Lines 101 to 111 in 4cb7a84

const config = getJavaConfiguration();
const exclusions: string[] = config.get<string[]>("import.exclusions", []);
const patterns: string[] = [];
for (const exclusion of exclusions) {
if (exclusion.startsWith("!")) {
continue;
}
patterns.push(exclusion);
}
return parseToStringGlob(patterns);

@gluxon gluxon force-pushed the exclude-node-modules-when-searching branch from 9525c16 to e03fa47 Compare October 24, 2023 06:11
@gluxon
Copy link
Contributor Author

gluxon commented Oct 24, 2023

@jdneo Is the recommendation to read the java.import.exclusions config and use it as the exclusionGlob here?

If so, I'd just want to double check a quick question before making changes in this PR. I think I could see a developer wanting certain .java files in java.import.exclusions to still be a trigger that activates the language server, but not be included in imports?

Thanks for the typo catch. Fixed in latest push.

@jdneo
Copy link
Collaborator

jdneo commented Oct 24, 2023

I think I could see a developer wanting certain .java files in java.import.exclusions to still be a trigger that activates the language server, but not be included in imports?

The extension will always be activated if a Java file is opened, which is defined here: https://github.com/redhat-developer/vscode-java/blob/master/package.json#L42

My understanding of this case is: The trigger files is used as an indicator of the project importer: https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/7e07649d7a9aee1872e5494b19e650058a577f55/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/InvisibleProjectImporter.java#L735. If the user has already added a path into that setting, it means the user does not want to import anything from that folder. So, using that setting should be ok.

@gluxon gluxon force-pushed the exclude-node-modules-when-searching branch from e03fa47 to 40916db Compare October 24, 2023 18:26
@gluxon
Copy link
Contributor Author

gluxon commented Oct 24, 2023

Thanks for the help @jdneo! The latest push now reuses the getExclusionsBlob() function. I attempted to describe why this change makes sense as a comment. Would appreciate a double check that comment is correct.

As for testing, I double checked this code path still finds a Test.java file at the root of the project if build.gradle is opened.

Screenshot 2023-10-24 at 2 26 16 PM
Screenshot 2023-10-24 at 2 26 22 PM

src/extension.ts Outdated Show resolved Hide resolved
@gluxon gluxon force-pushed the exclude-node-modules-when-searching branch from 40916db to 5c2a866 Compare October 25, 2023 18:52
Copy link
Collaborator

@jdneo jdneo left a comment

Choose a reason for hiding this comment

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

LGTM!

@rgrunber rgrunber merged commit 9f123e9 into redhat-developer:master Oct 26, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants