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

Jump to bottom

Feature/workspace/extension folder #2265

Merged
merged 2 commits into from
May 14, 2024
Merged

Feature/workspace/extension folder #2265

merged 2 commits into from
May 14, 2024

Conversation

KutluOzel-b
Copy link
Contributor

@KutluOzel-b KutluOzel-b commented Apr 3, 2024 •
edited
Loading

Changed copybook download folder from {workspace} /.c4z/.copybooks/... into {global storage folder}/.zowe/.copybooks/...

added 'open internal copybook folder command' to add copybook download folder to open workspaces

How Has This Been Tested?

Apply copybook settings as regular and try it out. nothing specific needed to test this.

Checklist:

  • Each of my commits contains one meaningful change
  • I have performed rebase of my branch on top of the development
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have checked my code and corrected any misspellings

slavek-kucera
slavek-kucera reviewed Apr 4, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/services/util/FSUtils.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/copybook/CopybookURI.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/copybook/CopybookURI.ts Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/copybook/CopybookDownloadService.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/LanguageClientService.ts Outdated Show resolved Hide resolved
...ts/cobol-lsp-vscode-extension/src/__tests__/services/copybook/CopybookResolveURITest.spec.ts Outdated Show resolved Hide resolved
...bol-lsp-vscode-extension/src/__tests__/services/copybook/CopybookDownloadServiceTest.spec.ts Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/__tests__/services/SettingsTest.spec.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/__tests__/services/LanguageClientServiceTest.spec.ts Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/package.json Outdated Show resolved Hide resolved
ishche
ishche requested changes Apr 8, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/extension.ts Outdated
@@ -55,6 +58,9 @@ function initialize() {
outputChannel = vscode.window.createOutputChannel("COBOL Language Support");
languageClientService = new LanguageClientService(outputChannel);
const configurationWatcher = new ConfigurationWatcher();
if (!fs.existsSync(Utils.getExtensionFolder())) {
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 that the creation of the filter on startup is a good idea. It should be created on demand.

Copy link
Contributor

@slavek-kucera slavek-kucera Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The copybook folder command might get more complicated, if the folder does not always exist... But I am ok with either way.

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 is changed to c64a4ff#diff-78d7e07f52d474dc72991f081f587e1f7aa1304d5b58996718648ae78c571cceL75

which is already created with activation.

clients/cobol-lsp-vscode-extension/src/extension.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/LanguageClientService.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/copybook/CopybookMessageHandler.ts
@@ -60,7 +63,7 @@ function searchCopybook(
dialectType,
);
const allowedExtensions = resolveAllowedExtensions(folderKind, documentUri);
result = searchCopybookInWorkspace(
result = searchCopybookInExtensionFolder(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we backward compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The copybooks that are stored under {workspace} /.cz4 is going to be redownloaded after resolving fails if provided settings are valid as before.

Do you see any other possible issues ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is with copybooks that are part of the workspace not the downloaded ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you set it with cobol-lsp.cpy-manager.paths-local setting. It will search for copybooks firstly from the provided path.

Do you mean not to ask to user to provide settings for copybooks local path ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is working. only if you provide the absolute path, which I think it is a good idea
For sake of backwards compatibility ;
This is resolved at : a5652c2

But the backwards logic wasn't a good idea. Why are we asking local path if we are already looking into workspace folder already ? User should be able to point a path if we ask to provide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Both relative (w.r.t. to workspace) and absolute path should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be working with : a5652c2

@ishche
Copy link
Contributor

ishche commented Apr 8, 2024

Please also fix integration tests and squash commits

@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from cb421c8 to e938f32 Compare April 10, 2024 10:57
@KutluOzel-b KutluOzel-b requested a review from ishche April 10, 2024 11:02
slavek-kucera
slavek-kucera reviewed Apr 11, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/__tests__/extensionTest.spec.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/__tests__/services/SettingsTest.spec.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/__tests__/services/SettingsTest.spec.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/Settings.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/Settings.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/services/util/FSUtils.ts
Comment on lines -87 to +30
export function searchCopybookInWorkspace(
export function searchCopybookInExtensionFolder(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that neither of these names is really appropriate...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any suggestions ? maybe searchCopybookInInternalFolder ?

Copy link
Contributor

@slavek-kucera slavek-kucera Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure, the function does too much stuff.

clients/cobol-lsp-vscode-extension/src/services/util/Utils.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/__tests__/commands/ClearCopybookCacheCommand.spec.ts Outdated Show resolved Hide resolved
@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from 7901de4 to 8c441f8 Compare April 11, 2024 13:16
@slavek-kucera
Copy link
Contributor

I think that the relative paths in the user settings should not be used in combination with the new download directory - please add a test for this situation.

@KutluOzel-b
Copy link
Contributor Author

I think that the relative paths in the user settings should not be used in combination with the new download directory - please add a test for this situation.

added test for this : 04536b2

slavek-kucera
slavek-kucera approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@slavek-kucera slavek-kucera left a comment

Choose a reason for hiding this comment

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

Pending testing.

@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from eda902c to 1c8c9ee Compare April 19, 2024 14:10
@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from 4160e84 to 2e0d54d Compare April 22, 2024 07:21
@ishche
Copy link
Contributor

ishche commented Apr 22, 2024

Could you please resolve conflicts, clean up the PR description, and squash commits?

@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from 57ee20a to 4043fc0 Compare April 22, 2024 09:19
slavek-kucera
slavek-kucera requested changes Apr 25, 2024
View reviewed changes
clients/cobol-lsp-vscode-extension/src/extension.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/extension.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/constants.ts Outdated Show resolved Hide resolved
clients/cobol-lsp-vscode-extension/src/extension.ts Outdated Show resolved Hide resolved
@ap891843 ap891843 mentioned this pull request May 7, 2024
Merged
9 tasks
@KutluOzel-b KutluOzel-b force-pushed the feature/workspace/extension_folder branch from a0c387c to 4b848b6 Compare May 13, 2024 13:43
@slavek-kucera slavek-kucera merged commit b3b4d33 into eclipse-che4z:development May 14, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slavek-kucera slavek-kucera slavek-kucera approved these changes

@ishche ishche Awaiting requested review from ishche

@VitGottwald VitGottwald Awaiting requested review from VitGottwald

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KutluOzel-b @ishche @slavek-kucera @VitGottwald