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

Update save.py to use utf-8 instead of None per default #3278

Merged
merged 12 commits into from
Dec 6, 2023

Conversation

franzhaas
Copy link
Contributor

I experience issues with the Null on windows. It appears as if when saving HTML, the template is already utf-8 and when writing the files with the system default code page, there are encoding issues.

Alternatively I see another options to solve my problem.:

set it only in the

elif format == "html":

branch...

I experience issues with the Null on windows. It appears as if when saving HTML, the template is already utf-8 and when writing the files with the system default code page, there are encoding issues.
@jonmmease
Copy link
Contributor

Thanks for the contribution @franzhaas. Setting the default encoding to utf-8 when saving our text-based formats (svg and html) makes sense. The tests are failing because it's not valid to set an encoding when saving to binary formats (png and pdf).

So I think you'll need to move the default encoding up a level and set it for only the svg and html branches.

@franzhaas
Copy link
Contributor Author

I think it's now ok.

...I need to get into the habit of using my account to run CI...

Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Thanks @franzhaas,

While reviewing, I noticed that for svg and vega formats, we look for an encoding kwarg and then fall back to utf-8 if it's not present. I think this makes sense for json and html formats as well. I added suggestions, so I added review suggestions that make this change.

Not a requirement for this PR, but it would be good to add encoding as an explicit argument to save with a default value of utf-8 and add it to the docstring. Right now it's basically a secret keyword argument.

for consistence with svg export, I added suggestions to honor an encoding argument passed into the function

altair/utils/save.py Outdated Show resolved Hide resolved
altair/utils/save.py Outdated Show resolved Hide resolved
@franzhaas
Copy link
Contributor Author

I think it is ok now...

I personally would prefer to not have the encoding parameter explicit, working by default and the ability to pass an IO object when specific encodings are necessary appear to be a good and compact solution too me.

@jonmmease
Copy link
Contributor

jonmmease commented Dec 6, 2023

Thanks @franzhaas, one last formatting fix needed to get the tests passing and this is good to go!

Edit: sorry, I posted and deleted a comment here that was meant for another thread

- fixed formating... wired...
@franzhaas
Copy link
Contributor Author

I am a bit confused about that one... I cant remember to have touched this file... and i see nothing in the logs... also it didn't trigger on my own CI run...

but sure i fixed it...

@jonmmease
Copy link
Contributor

Thanks! Maybe ruff released an update.

@jonmmease jonmmease merged commit 6d1ded7 into vega:main Dec 6, 2023
10 checks passed
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.

2 participants