-
Notifications
You must be signed in to change notification settings - Fork 36
GH-375 Fixed issue with encodings on USS #380
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #380 +/- ##
==========================================
+ Coverage 81.88% 81.99% +0.10%
==========================================
Files 48 48
Lines 2771 2893 +122
==========================================
+ Hits 2269 2372 +103
- Misses 502 521 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
t1m0thyj
left a comment
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.
Apologies for the delayed review, thanks @KUGDev for your contribution! 😁
Have a few questions related to how receive_encoding works in the download method, otherwise LGTM!
|
Hi @t1m0thyj. |
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
b4dcc68 to
4a784bc
Compare
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
t1m0thyj
left a comment
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 @KUGDev for making updates! Left one more comment.
I think the CI failures were due to a transient Codecov issue, as they passed when re-run.
| if not isinstance(response, requests.Response): | ||
| raise TypeError(f"Expected requests.Response, got {type(response)}") | ||
| with open(output_file, "wb" if binary else "w", encoding="utf-8") as f: | ||
| with open(output_file, "wb" if binary else "w", encoding=receive_encoding) as f: |
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.
When the open method is invoked with wb mode, it will throw an error if encoding argument is provided.
| with open(output_file, "wb" if binary else "w", encoding=receive_encoding) as f: | |
| with open(output_file, "wb" if binary else "w", encoding=None if binary else receive_encoding) as f: |
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.
Ah, good catch @t1m0thyj ! Thank you:)
I also noticed that in the data sets part the same functionality is implemented as download + download_binary. I don't know if it is ok to combine them the same way as in the USS part since some SDK users could already utilize that as it is right now. What do you think?
As a workaround I can propose to mark download_binary as deprecated.
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.
Also good catch on the inconsistency between USS and Data Set APIs 😋
I think combining the 2 methods for data sets into 1 makes sense, as long as we keep the old method deprecated 👍
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
b798e87 to
b417455
Compare
…upload for USSFiles and Datasets classes Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
b417455 to
65c708d
Compare
|
@t1m0thyj sorry, got too deep into changes😅 |
Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
…Response Signed-off-by: Uladzislau <leksilonchikk@gmail.com>
What It Does
Fixes #375
How to Test
uss.get_content("/path/to/file", file_encoding="UTF-8", receive_encoding="UTF-8"),uss.get_content_streamed("/path/to/file", binary=False, file_encoding="UTF-8", receive_encoding="UTF-8"),uss.download("/path/to/file", "/output/path", binary=False, file_encoding="UTF-8", receive_encoding="UTF-8")Review Checklist
I certify that I have:
Additional Comments