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

Refactor Files class into proper classes #274

Closed
wants to merge 3 commits into from
Closed

Refactor Files class into proper classes #274

wants to merge 3 commits into from

Conversation

pem70
Copy link
Collaborator

@pem70 pem70 commented May 30, 2024

What It Does
Refactor files.py into separate files. Class Files will be supported still, and each module can run independently.

How to Test

Review Checklist
I certify that I have:

Additional Comments

Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 70.95116% with 113 lines in your changes missing coverage. Please review.

Project coverage is 86.54%. Comparing base (5816aab) to head (ed5fde7).
Report is 230 commits behind head on main.

Files with missing lines Patch % Lines
.../zos_files/zowe/zos_files_for_zowe_sdk/datasets.py 71.96% 60 Missing ⚠️
src/zos_files/zowe/zos_files_for_zowe_sdk/uss.py 40.00% 36 Missing ⚠️
src/zos_files/zowe/zos_files_for_zowe_sdk/files.py 61.36% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #274      +/-   ##
==========================================
+ Coverage   86.21%   86.54%   +0.33%     
==========================================
  Files          35       38       +3     
  Lines        2292     2386      +94     
==========================================
+ Hits         1976     2065      +89     
- Misses        316      321       +5     
Flag Coverage Δ
unittests 86.54% <70.95%> (+0.33%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Signed-off-by: pem70 <[email protected]>
@pem70 pem70 marked this pull request as ready for review May 30, 2024 14:20
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.

LGTM @pem70 thanks for splitting these up into classes and updating the deprecated functions! 👍
I see that the patch coverage is a bit below the 80% minimum, but no worries about this as the other pull request is related and implements more patch coverage for that work 😋

@zFernand0 zFernand0 marked this pull request as draft June 3, 2024 19:20
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.

Changes LGTM! 😋
I like the direction that was taken with the refactoring.
Left two minor comments, both of which can be addressed in a new PR that combines all 3 issues:

CHANGELOG.md Show resolved Hide resolved
@zFernand0
Copy link
Member

Closing in favor of the next PR in line:

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.

3 participants