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: Introduce integrated terminal #3079

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

feat: Introduce integrated terminal #3079

wants to merge 65 commits into from

Conversation

zFernand0
Copy link
Member

@zFernand0 zFernand0 commented Aug 29, 2024

Proposed changes

Introduces the MVS, TSO, and Unix command handlers as integrated terminals

Release Notes

Milestone: 3.1.0

Changelog: Introduces the MVS, TSO, and Unix command handlers as integrated terminals

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Further comments

Merged and published:

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

Attention: Patch coverage is 93.38677% with 33 lines in your changes missing coverage. Please review.

Project coverage is 93.23%. Comparing base (aea6d92) to head (c998f8d).

Files with missing lines Patch % Lines
...s/zowe-explorer/src/commands/UnixCommandHandler.ts 88.23% 10 Missing ⚠️
...es/zowe-explorer/src/commands/TsoCommandHandler.ts 78.37% 8 Missing ⚠️
...es/zowe-explorer/src/commands/MvsCommandHandler.ts 84.37% 5 Missing ⚠️
.../zowe-explorer/src/commands/ZoweCommandProvider.ts 96.55% 4 Missing ⚠️
...-explorer-api/src/profiles/ZoweExplorerZosmfApi.ts 62.50% 3 Missing ⚠️
packages/zowe-explorer/src/tools/ZoweTerminal.ts 98.95% 2 Missing ⚠️
...ckages/zowe-explorer/src/configuration/Profiles.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3079      +/-   ##
==========================================
+ Coverage   93.17%   93.23%   +0.05%     
==========================================
  Files         117      118       +1     
  Lines       12277    12393     +116     
  Branches     2822     2856      +34     
==========================================
+ Hits        11439    11554     +115     
- Misses        837      838       +1     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zFernand0 zFernand0 self-assigned this Aug 29, 2024
@adam-wolfe adam-wolfe added this to the v3.1.0 milestone Sep 3, 2024
@zFernand0 zFernand0 mentioned this pull request Sep 5, 2024
15 tasks
Copy link

sonarqubecloud bot commented Sep 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
22.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Signed-off-by: Fernando Rijo Cedeno <[email protected]>
…nto feat/int-term

Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
@awharn
Copy link
Member

awharn commented Nov 25, 2024

One thing I did notice was that if I attempt to Ctrl+C without thinking about it, the terminal gets closed. I am not sure if we can do anything about that, but it is something that we might want to address if we can. Perhaps try to close the last command's address space if we can? And if the command hasn't been run, but someone started typing it, clear the command?

…nto feat/int-term

Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
@JTonda JTonda marked this pull request as draft November 26, 2024 16:10
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Copy link
Member Author

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Addressing some comments:

  • All input is blocked when a command is running (except for ctrl+C to cancel the command)

package.json Outdated Show resolved Hide resolved
packages/zowe-explorer/src/commands/ZoweCommandProvider.ts Outdated Show resolved Hide resolved
packages/zowe-explorer/src/commands/ZoweCommandProvider.ts Outdated Show resolved Hide resolved
@zFernand0 zFernand0 dismissed stale reviews from JillieBeanSim and traeok December 10, 2024 20:57

Changes have been addressed 🙏

Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
@adam-wolfe
Copy link
Contributor

Something after b707bdb seems to have broken my ability to launch an integrated terminal. Instead, it takes me to the quick pick for entering a TSO command.

@zFernand0
Copy link
Member Author

Something after b707bdb seems to have broken my ability to launch an integrated terminal. Instead, it takes me to the quick pick for entering a TSO command.

The default setting for the value was changed to false, making this an opt-in feature that can be released at any time 🙏
image

adam-wolfe
adam-wolfe previously approved these changes Dec 12, 2024
Copy link
Contributor

@adam-wolfe adam-wolfe left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @zFernand0. Will re-review/approve when out of draft.

@traeok traeok self-requested a review December 18, 2024 15:03
Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all the feedback Fernando 😋 I had a couple questions regarding behaviors that I noticed while using the terminal, but happy to approve once the PR is ready to go 😁


  • Environment variables don't seem to work in the context of Unix/SSH. We might want to warn users that environment variables cannot be set when issuing Unix commands as part of the integrated terminal doc. Do we want to handle this as a separate enhancement? Thanks for clarifying on the environment variables, I removed my examples because the first one should technically work with && between the two statements (env. var and the echo), and the second is improper syntax 😅
  • A few keys render as "replacement characters," sometimes with additional escape sequences afterward. Do we want to ignore these inputs or keep this behavior as-is? I tried the following sequences (in order of characters in screenshot):
    • Tab
    • Ctrl+Backspace
    • Page Up (rendered as �[5~)
    • Page Down (rendered as �[6~)
    • Ctrl+Left Arrow (shown twice, rendered as �[1;5D)
    • Ctrl+Right Arrow (rendered as �[1;5C)
      image

@zFernand0 zFernand0 marked this pull request as ready for review December 18, 2024 18:36
@zFernand0
Copy link
Member Author

Thanks @adam-wolfe and @traeok for testing.
I've just added a few more edge cases (with modifier keys) and made the "exit" approach the same as the node shell/interpreter 🙏
(To exit, press Ctrl+C again or Ctrl+D or type .exit) 😋

Trae, the ENV vars is a good catch.
As long as you split it in separate commands, it should work just fine.
Also, to resolve ENV vars, I believe echo requires "
MY_CUSTOM_VAR=world; echo "Hello $MY_CUSTOM_VAR"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

6 participants