-
Notifications
You must be signed in to change notification settings - Fork 92
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
Allow users to open data sets and USS files in another codepage #2594
Conversation
Think this also resolves #887. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @t1m0thyj for this work on enhancing encoding support 😋
Just left a couple comments on the localized strings and the openPS
function:
packages/zowe-explorer/__tests__/__integration__/dataset/actions.integration.test.ts
Fixed
Show fixed
Hide fixed
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2594 +/- ##
==========================================
+ Coverage 93.15% 93.24% +0.09%
==========================================
Files 101 102 +1
Lines 10323 10471 +148
Branches 2183 2237 +54
==========================================
+ Hits 9616 9764 +148
Misses 706 706
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Moved most of the refactoring into a separate PR (#2634) to hopefully make the encoding changes easier to review |
This reverts commit 3c2fb92. Signed-off-by: Timothy Johnson <[email protected]>
Signed-off-by: Timothy Johnson <[email protected]>
fe3ba24
to
124c0b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Tested with a dataset using a double byte encoding, and it works great! Only comment is that creating a profile or team config is starting to get a bit long via the UI, with a bunch of questions around encoding, certificates, APIML base path, port, protocol, scheme, timeouts, etc. Nothing against this PR, just an observation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation looks and works great! Thanks @t1m0thyj for this feature, I like the UX as well 😁
One thing that might be worth looking into is offering the "Open with Encoding" command in the Command Palette - it might involve some adjustments to the command itself, but would be a nice shortcut so users can quickly change the encoding of the active editor 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works great! thanks @t1m0thyj
The base branch was changed.
Signed-off-by: Billie Simmons <[email protected]>
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 5 New issues |
Proposed changes
ZoweLocalStorage
class from "next" branch to store recent codepage historyHow to Test
Release Notes
Milestone: 2.14.0
Changelog:
Types of changes
What types of changes does your code introduce to Zowe Explorer?
Put an
x
in the boxes that applyChecklist
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 revieweryarn workspace vscode-extension-for-zowe vscode:prepublish
has been executedFurther comments
ze-encoding.mp4