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.

Sign up for GitHub

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

Issue455 refresh issue due to pom generated in target on building the project #462

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+27 −13
Open

Issue455 refresh issue due to pom generated in target on building the project #462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b74ca37
Restricting the refresh for any changes in the build or target directory
Jan 2, 2025
bcebc27
excluding target and build folders for pom and build.gradle scans
Jan 3, 2025
b899512
Added build folder to excluded directories
Jan 3, 2025
f9b2bc0
Added check to avoid refresh when there are changes in target or buil…
Jan 3, 2025
64fb495
Added check for build files in target or build directories
Jan 8, 2025
4d5102f
code formatted
Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/definitions/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const COMMAND_AND_PROJECT_TYPE_MAP: { [command: string]: string[] } = {
"gradle":[ LIBERTY_GRADLE_PROJECT, LIBERTY_GRADLE_PROJECT_CONTAINER],
"liberty.dev.debug": [LIBERTY_MAVEN_PROJECT, LIBERTY_GRADLE_PROJECT, LIBERTY_MAVEN_PROJECT_CONTAINER, LIBERTY_GRADLE_PROJECT_CONTAINER],
};
export const EXCLUDED_DIR_PATTERN = "**/{bin,classes,target}/**";
export const EXCLUDED_DIR_PATTERN = "**/{bin,classes,target,build}/**";
Copy link
Contributor

Choose a reason for hiding this comment

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

What is EXCLUDED_DIR_PATTERN used for? I see that target was already included, so this must not be working for the refresh scenario. What is the reason for adding build here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In refresh method we are calling updateProjects() which will be looking into all the build files in the workspace . there we also mention the directories to exclude for this search and build directory was missing in EXCLUDED_DIR_PATTERN. since we want to avoid to looking into the build folder in case of gradle projects i included 'build' in EXCLUDED_DIR_PATTERN

export const COMMAND_TITLES = new Map();
export const UNTITLED_WORKSPACE="Untitled (Workspace)";
COMMAND_TITLES.set(localize("hotkey.commands.title.refresh"), "liberty.explorer.refresh");
Expand Down
30 changes: 18 additions & 12 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,8 @@ function registerCommands(context: ExtensionContext) {
devCommands.deleteTerminal(closedTerminal);
})
);
// Listens for any new folders are added to the workspace
context.subscriptions.push(vscode.workspace.onDidChangeWorkspaceFolders((event) => {
// Listens for any new folders are added to the workspace
context.subscriptions.push(vscode.workspace.onDidChangeWorkspaceFolders((event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not modify spacing or formatting on existing code, unless it is in a separate PR specific to formatting changes. It is not immediately clear if there are any functional changes in the middle of formatting changes, which makes code more difficult to review and makes it more difficult for others to understand when looking back at code history.

projectProvider.refresh();
}));
}
Expand All @@ -183,16 +183,22 @@ export function deactivate(): Promise<void[]> {
* @param projectProvider Liberty Dev projects
*/
export function registerFileWatcher(projectProvider: ProjectProvider): void {
const watcher: vscode.FileSystemWatcher = vscode.workspace.createFileSystemWatcher("{**/pom.xml,**/build.gradle,**/settings.gradle,**/src/main/liberty/config/server.xml}");
watcher.onDidCreate(async () => {
projectProvider.refresh();
});
watcher.onDidChange(async () => {
projectProvider.refresh();
});
watcher.onDidDelete(async () => {
projectProvider.refresh();
});
const watcher: vscode.FileSystemWatcher = vscode.workspace.createFileSystemWatcher("{**/pom.xml,**/build.gradle,**/settings.gradle,**/src/main/liberty/config/server.xml}");
const isInTargetOrBuild = (uri: vscode.Uri) => {
return helperUtil.isInTargetOrBuild(uri.fsPath);
}
if (!isInTargetOrBuild) {
watcher.onDidCreate(async (uri) => {
projectProvider.refresh();
});
watcher.onDidChange(async (uri) => {
projectProvider.refresh();
});
watcher.onDidDelete(async (uri) => {
projectProvider.refresh();
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not entirely sure about this approach, since we are still watching the "target" and "build" dirs. It would be better if we could find a way to ignore them completely.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add '&&' operator in vscode.workspace.createFileSystemWatcher?
Did conditions like this work '**/pom.xml && !inside target folder' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for glob patterns && operators are not available in their syntax, however negations are available as per their documentation
https://code.visualstudio.com/docs/editor/glob-patterns

Copy link
Contributor

@aparnamichael aparnamichael Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@JoshwinThomasIBM {} to group conditions (for example {**/*.pom.xml,!inside target folder} )

Can you try with this group conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aparnamichael i tried with "/!(target)//{pom.xml,build.gradle,settings.gradle,src/main/liberty/config/server.xml}
and it seems to be working , but multiple negations is not supported in glob pattern
also with the above glob patten we expect only the one's in target to be negated and other pom's generated to be watched ,but if the the pom is generated in some other location then that is not being caught by the watcher .

}

function startLangServer(context: ExtensionContext, requirements: RequirementsData, isLiberty: boolean) {
Expand Down
8 changes: 8 additions & 0 deletions src/util/helperUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,12 @@ export function clearDataSavedInGlobalState(context: vscode.ExtensionContext) {
context.globalState.update('workspaceSaveInProgress', false);
context.globalState.update('selectedProject', undefined);
}
/**
* Method checks if the given file path is in target or build folders and returns a boolean
* @param filePath
* @returns
*/
export function isInTargetOrBuild(filePath: string): boolean {
return filePath.includes('/target/') || filePath.includes('/build/');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is valid if it is just any "target" or "build" dir. We need to ensure the file/path is in a subdirectory of the project's "target" or "build" directory (i.e. the "target" or "build" directory located in the root project folder).

}

Loading