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.

Sign up for GitHub

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

Jump to bottom

validate existing type annotations #353

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

validate existing type annotations #353

wants to merge 1 commit into from

Conversation

olamidepeterojo
Copy link
Contributor

Fixes #321

How to Test

Review Checklist
I certify that I have:

Additional Comments

traeok
traeok requested changes Feb 26, 2025
View reviewed changes
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 adding all these types! I had a question about one parameter and a concern about interpreting binary contents.

src/core/zowe/core_for_zowe_sdk/profile_manager.py Outdated
@@ -67,11 +67,11 @@ class ProfileManager:
"""

def __init__(self, appname: Optional[str] = "zowe", show_warnings: Optional[bool] = True):
Copy link
Member

@traeok traeok Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If appname is Optional[str] but has a default value, could the type be updated to str? If not feel free to disregard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it can, thanks for pointing this out. I've updated the type from Optional[str] to str

src/zos_files/zowe/zos_files_for_zowe_sdk/uss.py Outdated
return {
"content_type": response.headers.get("Content-Type"),
"content_length": len(response.content),
"file_content": response.content.decode("utf-8", errors="ignore") # You can also base64-encode this if needed
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it could break output for binary files because even though the content is binary, we try to decode the response content as UTF-8.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is true. we could avoid directly decoding binary content as UTF-8, so i base64-encode the binary data.

zFernand0
zFernand0 requested changes Mar 3, 2025
View reviewed changes
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.

the overall goal makes sense to me 😋

However, there are a few things that I'm not sure that's exactly they should be done. 😋

Left some comments since I'm not able to run unit (nor integration) tests successfully in this PR 😢

src/zos_console/zowe/zos_console_for_zowe_sdk/response/__init__.py Outdated
Comment on lines 13 to 16
from .console import ConsoleResponse, IssueCommandResponse
from .console_response import ConsoleResponse
from .issue_response import IssueCommandResponse

__all__= ["ConsoleResponse", "IssueCommandResponse"]
Copy link
Member

Choose a reason for hiding this comment

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

I won't claim to be an expert on how this all works, but where is .issue_response?
I'm afraid I can't get the unit tests to run because of this (and many similar changes) 🤔

src/core/zowe/core_for_zowe_sdk/sdk_api.py Outdated
@@ -17,6 +17,7 @@
from .logger import Log
from .request_handler import RequestHandler
from .session import ISession, Session
from typig import Dict, Any, Optional, Type, Union
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from typig import Dict, Any, Optional, Type, Union
from typing import Dict, Any, Optional, Type, Union

src/zos_files/zowe/zos_files_for_zowe_sdk/response/__init__.py Outdated
@@ -11,4 +11,6 @@
"""
from .datasets import DatasetListResponse, MemberListResponse
from .file_system import FileSystemListResponse
from .uss import USSListResponse
from .uss_response import USSListResponse
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from .uss_response import USSListResponse
from .uss import USSListResponse

src/zos_jobs/zowe/zos_jobs_for_zowe_sdk/response/__init__.py
Comment on lines -12 to +14
from .jobs import JobResponse, SpoolResponse, StatusResponse
from .job_response import JobResponse
from .spool_response import SpoolResponse
from .status_response import StatusResponse
Copy link
Member

Choose a reason for hiding this comment

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

I believe we might want to refer these changes 😋

zFernand0
zFernand0 requested changes Mar 5, 2025
View reviewed changes
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.

Seems like some items have not been resoled yet.
Please check them out before re-requesting review 🙏

@olamidepeterojo
validate existing type annotations
ef18862 
Signed-off-by: Olamide Ojo <peterojoolamide@gmail.com>
@zFernand0
Copy link
Member

Not sure if the re-request is happening automatically because of the force-push 😓
However, I still see a few issues that are not resolved. 😅

For now, I'll remove myself from the list of reviewers on this PR 🙏

@zFernand0 zFernand0 removed their request for review March 5, 2025 14:56
@olamidepeterojo
Copy link
Contributor Author

@zFernand0 my apologies. haha, ive been the one re-requesting review. could you please point out the few issues you noticed cause the ones mentioned earlier has been solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zFernand0 zFernand0 zFernand0 requested changes

@t1m0thyj t1m0thyj Awaiting requested review from t1m0thyj

@traeok traeok Awaiting requested review from traeok

Requested changes must be addressed to merge this pull request.

Assignees

@olamidepeterojo olamidepeterojo

Labels
None yet
Projects
Zowe CLI Squad
Status: Review/QA
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate type hints and fix any incorrect ones
3 participants
@olamidepeterojo @zFernand0 @traeok