-
Notifications
You must be signed in to change notification settings - Fork 28
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
Default encoding for I/O operations should be UTF-8 #244
Conversation
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: Timothy Johnson <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #244 +/- ##
=======================================
Coverage 83.75% 83.75%
=======================================
Files 34 34
Lines 2118 2118
=======================================
Hits 1774 1774
Misses 344 344
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@aadityasinha-dotcom Downloading spool files with binary content works now 👍
Could you please do the following:
- Update the changelog to mention the issue that was fixed
- Add
encoding="utf-8"
to all the places whereopen()
is used in the SDK except for cases that use binary mode (update all the calls that user
andw
mode, but notrb
andwb
)
Signed-off-by: aadityasinha-dotcom <[email protected]>
…e-client-python-sdk into encoding Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
@@ -266,7 +266,7 @@ def submit_from_local_file(self, jcl_path): | |||
A JSON containing the result of the request execution | |||
""" | |||
if os.path.isfile(jcl_path): | |||
jcl_file = open(jcl_path, "r") | |||
jcl_file = open(jcl_path, "r", encoding="utf-8") |
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.
Added encoding="utf-8" for all
open()`
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.
Changes LGTM! 😋
but there is a merge conflict committed in the CHANGELOG.md file 😋
CHANGELOG.md
Outdated
<<<<<<< HEAD | ||
- Bug: Default encoding for I/O operations should be UTF-8 | ||
- Feature: Added method to load profile properties from environment variables | ||
======= |
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.
there is still a merge conflict here 😋
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.
Yes my bad, I forgot to resolve them 😋
It should be good now
Signed-off-by: aadityasinha-dotcom <[email protected]>
Signed-off-by: aadityasinha-dotcom <[email protected]>
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.
LGTM, thanks @aadityasinha-dotcom!
Signed-off-by: Timothy Johnson <[email protected]>
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.
LGTM! 😋
Release succeeded for the The following packages have been published:
Powered by Octorelease 🚀 |
What It Does
How to Test
Review Checklist
I certify that I have:
Additional Comments