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

fix: add extensions back to Data Set resources; resolve USS copy/paste bug #3127

Merged
merged 22 commits into from
Sep 23, 2024

Conversation

traeok
Copy link
Member

@traeok traeok commented Sep 19, 2024

Proposed changes

  • Fixed issue where file extensions were removed from data sets, causing language detection to sometimes fail for Zowe Explorer extenders. #3121
  • Fixed an issue where copying and pasting a file/folder in the USS tree would fail abruptly, displaying an error. #3128

Release Notes

Milestone: vNext

Changelog: See Proposed changes

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):

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok changed the title wip(fix): add extensions back to data set resources wip(fix): add extensions back to Data Set resources Sep 19, 2024
Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.55%. Comparing base (45fb36f) to head (e250473).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
...we-explorer/src/trees/dataset/DatasetFSProvider.ts 95.83% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3127      +/-   ##
==========================================
- Coverage   92.60%   92.55%   -0.05%     
==========================================
  Files         113      113              
  Lines       11657    11649       -8     
  Branches     2591     2442     -149     
==========================================
- Hits        10795    10782      -13     
- Misses        860      865       +5     
  Partials        2        2              

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

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok changed the title wip(fix): add extensions back to Data Set resources fix: add extensions back to Data Set resources Sep 19, 2024
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Copy link

github-actions bot commented Sep 19, 2024

📅 Suggested merge-by date: 10/3/2024

@phaumer
Copy link
Member

phaumer commented Sep 19, 2024

Why are even the file extensions required? If a user stores a JCL file in a data set that has COB or CBL in the name Zowe Explorer automatically assumes it is a COBOL file. If we leave Zowe Explorer agnostic then the user can define their own rules in "file.associations" using glob patterns there. Is there a limitation in the new virtual file system that requires using extensions?

I know we had this in v1 and v2, but now is the opportunity to remove it (perhaps with a switch to turn it back on).

@traeok
Copy link
Member Author

traeok commented Sep 19, 2024

Why are even the file extensions required? If a user stores a JCL file in a data set that has COB or CBL in the name Zowe Explorer automatically assumes it is a COBOL file. If we leave Zowe Explorer agnostic then the user can define their own rules in "file.associations" using glob patterns there. Is there a limitation in the new virtual file system that requires using extensions?

I know we had this in v1 and v2, but now is the opportunity to remove it (perhaps with a switch to turn it back on).

@phaumer They aren't necessarily required, but without the extensions, it means that all users must define file associations for their data sets in order to get language detection - unless the LSP itself supports automatic detection. Adding a file extension allows VS Code to determine which LSPs are viable. Alternatively, we can assign a language ID, but since we can't assume/prioritize one language ID over another, the team felt it made sense to leverage the current behavior exhibited in v2 (adding file extensions).

I've tested Zowe Explorer with all extension suffixes & language detection removed, but this causes some data sets to open without any language detection (detected as "Plain Text"). This would be a regression from v2's behavior, as the file extension provided context to VS Code so it can refer to those LSPs. By removing all aspects of this detection, it would shift the responsibility to LSPs to perform the detection (as well as requiring users to add their own associations).

Ideally, we can get rid of this altogether - but this would be a breaking change for both users and extenders, and we are unfortunately past that cutoff for the v3 milestone.

@phaumer
Copy link
Member

phaumer commented Sep 19, 2024

@phaumer They aren't necessarily required, but this means that all users must define file associations for their data sets in order to get language detection unless the LSP itself supports automatic detection. Adding a file extension allows VS Code to determine which LSPs are viable.

Yes, it should be down to the extenders providing the actual language support to make that determination imho. In Z Open Editor we generate files.associations based on our user settings and would not want Zowe Explorer make these decisions for us.

To make it less breaking we could provide a switch to re-enable this (for users who want to continue using it) and could even add an extension point in the SDK to inject name manipulation code on open for extenders, but the current behavior introduced language specific behavior that language extenders might not want (which I complained about already for v1 :-) ).

@traeok
Copy link
Member Author

traeok commented Sep 19, 2024

To make it less breaking we could provide a switch to re-enable this (for users who want to continue using it) and could even add an extension point in the SDK to inject name manipulation code on open for extenders, but the current behavior introduced language specific behavior that language extenders might not want (which I complained about already for v1 :-) ).

I think it makes sense to add a switch, but since the cut-off for breaking changes in Zowe v3 was the end of August, we'll have to keep this default behavior in place. As v3 progresses, we can look into an enhancement that would allow users to opt out of this behavior.

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok
Copy link
Member Author

traeok commented Sep 19, 2024

Okay, should be good to go now - please disregard the SonarCloud warning for duplication as it only occurs in test code.

@traeok traeok marked this pull request as ready for review September 19, 2024 22:14
@traeok traeok requested a review from zFernand0 September 19, 2024 22:14
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@traeok traeok requested a review from anaxceron September 19, 2024 22:16
@phaumer
Copy link
Member

phaumer commented Sep 19, 2024

I understand. I created #3131 for the follow-up.

@benjamin-t-santos
Copy link
Contributor

I tested out opening data sets - extensions are being added like they were in v2 👍

anaxceron
anaxceron previously approved these changes Sep 20, 2024
@traeok
Copy link
Member Author

traeok commented Sep 20, 2024

I tested out opening data sets - extensions are being added like they were in v2 👍

Great - thanks for testing!

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
@JillieBeanSim JillieBeanSim mentioned this pull request Sep 23, 2024
15 tasks
Signed-off-by: Billie Simmons <49491949+JillieBeanSim@users.noreply.github.com>
Copy link
Member

@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.

LGTM! 😋
Left a few questions, mostly for my own understanding.
I don't believe that any of my comments should require changes to be made.

zFernand0
zFernand0 previously approved these changes Sep 23, 2024
Copy link
Member

@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.

Small merge conflict, but LGTM! 😋

Signed-off-by: Trae Yelovich <trae.yelovich@broadcom.com>
Copy link
Contributor

@JillieBeanSim JillieBeanSim left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @traeok

Copy link

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

Copy link
Member

@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.

LGTM! 😋

@JillieBeanSim JillieBeanSim merged commit 3c524b8 into main Sep 23, 2024
30 of 31 checks passed
@JillieBeanSim JillieBeanSim deleted the fix/add-ds-extensions branch September 23, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed
7 participants