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

Update environment check for Theia compatability #1010

Merged
merged 6 commits into from
Sep 11, 2020

Conversation

lauren-li
Copy link
Contributor

Proposed changes

Fixes issue #1009 and restores general Theia compatability.

Release Notes

Milestone: 1.9.0

Changelog: Update environment check for Theia compatability.

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • npm run vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

…tionName as env.appName

Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
@lauren-li lauren-li requested a review from phaumer September 8, 2020 17:59
@lauren-li lauren-li marked this pull request as ready for review September 8, 2020 18:19
phaumer
phaumer previously approved these changes Sep 9, 2020
Copy link
Member

@phaumer phaumer left a comment

Choose a reason for hiding this comment

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

I tested this fix successfully in VS Code, Theia 1.3, and CRW 2.3 (which is Che 7.16.2)

@phaumer phaumer added this to the 1.9 Release milestone Sep 9, 2020
crawr
crawr previously approved these changes Sep 9, 2020
Copy link
Contributor

@crawr crawr 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 fixing this. I wonder if instead of hard-coding the value "2", will vscode detect the variable vscode.UIKind.Web instead?

I tested several of the Theia branching code that we have (copy path, merge conflict, filter prompt, etc) and they all worked as expected.

@lauren-li
Copy link
Contributor Author

@crawr Yes, vscode.UIKind.Web does work. Let's use that instead.

(I didn't use it earlier because my capitalization was uiKind instead of UIKind when I was trying this, and thus it wasn't working for me. Thanks for pointing this out!)

Signed-off-by: Lauren Li <45975633+lauren-li@users.noreply.github.com>
@lauren-li lauren-li dismissed stale reviews from crawr and phaumer via 64a6d2d September 9, 2020 19:33
@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #1010 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1010   +/-   ##
=======================================
  Coverage   91.61%   91.61%           
=======================================
  Files          50       50           
  Lines        4830     4830           
  Branches      852      852           
=======================================
  Hits         4425     4425           
  Misses        403      403           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 027db5e...99dfe31. Read the comment docs.

@lauren-li
Copy link
Contributor Author

@crawr @phaumer Thank you for testing out this PR! I have updated it according to @crawr's suggestion, and it is ready to be reviewed again.

Copy link
Member

@phaumer phaumer left a comment

Choose a reason for hiding this comment

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

Retested and it still works.

Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

LGTM!

@zFernand0 zFernand0 merged commit 9100ee3 into master Sep 11, 2020
@zFernand0 zFernand0 deleted the theia-v1.1.0-update-fix branch September 11, 2020 19:17
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