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

SNOW-686222: Remove os.path.realpath in storage_client.py #1326

Merged
merged 1 commit into from
Nov 11, 2022

Conversation

sfc-gh-jdu
Copy link
Collaborator

Please answer these questions before submitting your pull requests. Thanks!

  1. What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-686222

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am modifying authorization mechanisms
    • I am adding new credentials
    • I am modifying OCSP code
    • I am adding a new dependency
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-686222-fix-realpath branch from e5bb07f to b6245a4 Compare November 9, 2022 22:41
tox.ini Outdated
@@ -181,7 +181,7 @@ known_first_party =snowflake,parameters,generate_test_files
[flake8]
# Notes on ignores:
# - all ignored Ds mean doc issues, these should be cleaned up
ignore = B011,C901,D100,D101,D102,D103,D104,D105,D107,D401,E203,E402,E501,F821,W503
ignore = B011,B027,C901,D100,D101,D102,D103,D104,D105,D107,D401,E203,E402,E501,F821,W503
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is needed for

def _update_presigned_url(self) -> None:
pass
# Override in S3
def _initiate_multipart_upload(self) -> None:
pass
# Override in S3
def _complete_multipart_upload(self) -> None:
pass
# Override in S3
def _abort_multipart_upload(self) -> None:
. Not all of them are implemented in subclasses, so adding @AbstractMethod will raise error. So let's just ignore this violation now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of ignoring this issue in the whole project, could we add comments on these definitions to ignore this error for only these functions?

Copy link
Collaborator Author

@sfc-gh-jdu sfc-gh-jdu Nov 10, 2022

Choose a reason for hiding this comment

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

I changed pass to return. I did try to add #noqa to ignore the flake8 violation, but yesqa keeps remove #noqa saying it's unnesscary 🤦 and I couldn't find a way to add #noqa here for this function...

@sfc-gh-jdu sfc-gh-jdu requested review from a team and sfc-gh-yixie and removed request for a team November 10, 2022 22:50
@sfc-gh-jdu sfc-gh-jdu force-pushed the jdu-SNOW-686222-fix-realpath branch from b6245a4 to e7f1b6a Compare November 10, 2022 23:48
Copy link
Collaborator

@sfc-gh-mkeller sfc-gh-mkeller left a comment

Choose a reason for hiding this comment

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

These classes could use a V2 sometime soon :)

🚢 🚢

@sfc-gh-jdu sfc-gh-jdu merged commit aa6c88f into main Nov 11, 2022
@sfc-gh-jdu sfc-gh-jdu deleted the jdu-SNOW-686222-fix-realpath branch November 11, 2022 18:34
@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants