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

feat: Create command to run analysis vscode command via CLI/terminal #2368

Conversation

mm-broadcom
Copy link
Contributor

@mm-broadcom mm-broadcom commented Jul 2, 2024

Adds a new command to the command palette such that the user can easily trigger the analysis from the CLI. Handles both the Java and Native versions of the server along with saved and unsaved files.

How Has This Been Tested?

Functionality:

  1. Open VS code with a cobol file open as the active file/editor
  2. Press F1 to open the command palette
  3. Type "cobol analysis" and select that command
  4. Select either "Java" or "Native"
  5. Press enter, see the output in the terminal window at the bottom. (Default UI location)

Tests created:

  • Cobol Analysis - Java: Tests when the user would have selected "Java" as the environment
  • Cobol Analysis - Native: Tests when the user would have selected "Native" as the environment
  • Cobol Analysis - Undefined Type: Tests when the user would have backed out of the command without selecting anything
  • Cobol Analysis - Save temp file: Tests what happens when the user tries to run the command on an unsaved file.
  • Various functions to test/determine functionality.

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

Initial analysis CLI command which runs uses the Java backend in the CLI.
Added prompting so the user can choose between java and native builds to run. Initial start on supporting copybook config files as well.
Added temp/unsaved file support.
Added documentation for the various functions that were created while developing this feature.
Add multi-step UI input allowing the user to cancel out without the code moving onwards.

Utilized a portion of Microsoft's vscode extension sample for QuickInput implementation. The license there is MIT, may need to add it to the extension's license file as well. Link: https://github.com/microsoft/vscode-extension-samples/tree/main/quickinput-sample
Fixed function access
Due to changes of what is being requested, copybook support was commented out.
Removed fs-temp, replaced it with the VSCodeAPI to handle the file system for writing temporary files elsewhere for analysis.
Initial, working, version of a unit test for the RunAnalysisCLI class.
Expanded tests with more "expect" calls and also support for when the user tries to call it with native functionality.
Simplified/streamlined the UI code due to no longer needing a copybook handling step. Added more unit tests.
Add unit test for saving a temporary file.
Fix build issue due to Prettier throwing an error.
Add more unit tests to increase coverage of branches and functions.
Ran prettier
Comment on lines 138 to 168
protected buildJavaCommand(currentFileLocation: string) {
const extensionFolder: string | undefined =
this.getExtensionPath() + "/server/jar/server.jar";

if (extensionFolder && currentFileLocation !== "") {
return (
'java -jar "' +
extensionFolder +
'" ' +
this.buildAnalysisCommandPortion(currentFileLocation)
);
}

return "";
}

/**
* Provides the portion of the command from "analysis" onwards.
* Is the same for both the Java and Native builds.
* @param currentFileLocation - Location of the main cobol file being analyzed.
* @protected
*/
protected buildAnalysisCommandPortion(currentFileLocation: string) {
const copyBookCommand = `-cf=${
this.copybookConfigLocation === ""
? "."
: '"' + this.copybookConfigLocation + '"'
}`;

return 'analysis -s "' + currentFileLocation + '" ' + copyBookCommand;
}
Copy link
Contributor

@slavek-kucera slavek-kucera Jul 3, 2024

Choose a reason for hiding this comment

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

How seriously do we want to handle escaping in the command sent to the shell? @VitGottwald
Backslashes, percentage in windows shell, both $(pwd) and `pwd` are valid filenames both on linux and windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

We generate the file name. So the only problem is the path to the folder provided by VS Code. And that depends on install location. So I would start with what is needed for standard install locations on windows/linux/mac.

Fixed saving temp files for analysis, addressed concerns raised in the PR, updated unit tests
Adds functionality to find or create a temp folder. If it exists, delete any files within. This is only ever used by the extension. Even in the result of the `context.globalStorageUri` being compromised, the inclusion of the `/tempAnalysisFiles` folder off of a given directory will stop unintended deletions.
Simplified the temp folder creation and improved the unit tests.
mm-broadcom and others added 3 commits July 8, 2024 08:20
Co-authored-by: slavek-kucera <53339291+slavek-kucera@users.noreply.github.com>
Wrap the map function used to delete files with a `promise.all`.
Cleaned up the tests to be more streamlined per suggestions on PR.
@ishche ishche removed their request for review July 10, 2024 08:03
Handle unsaved/remote files the same by saving the current text editor's contents to a local file first.
Copy link
Contributor

@VitGottwald VitGottwald left a comment

Choose a reason for hiding this comment

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

LGTM. Please, add the option to disable diagnostics in the CLI analysis in the PR where that option is added.

Thank you!

@mm-broadcom
Copy link
Contributor Author

Will do so once this PR is merged in. Should not take too long for me to implement. The PR Vit is referring to add the option to is #2379.

@VitGottwald VitGottwald merged commit eb2ae64 into eclipse-che4z:development Jul 11, 2024
16 checks passed
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.

4 participants