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 GT.save() #454

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Conversation

jrycw
Copy link
Collaborator

@jrycw jrycw commented Sep 18, 2024

Related issues: #391, #440

This PR aims to refine GT.save() in three areas:

  1. Use Path().suffix to check the file extension.
  2. Unpack window_size directly instead of using indexing.
  3. Add an encoding parameter, defaulting to utf-8 for writing temporary files.

I'm not entirely sure if utf-8 is the ideal fix for all users, but I believe it's the best default for most. Let's start with utf-8 and adjust over time if needed.

@jrycw
Copy link
Collaborator Author

jrycw commented Sep 18, 2024

I'm curious why codecov bot isn't running for this PR?

@rich-iannone
Copy link
Member

I'm curious why codecov bot isn't running for this PR?

I looked into our project configuration in the codecov site and saw nothing that requires attention. This might be a rare miss from codecov (maybe due to services being down). I'll see if this can be reported to them however.

@machow machow self-requested a review September 18, 2024 15:50
Copy link
Collaborator

@machow machow left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@rich-iannone rich-iannone merged commit 0af5fe4 into posit-dev:main Sep 18, 2024
11 checks passed
@jrycw jrycw deleted the encoding-issue-GT-save branch September 18, 2024 16:15
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.

3 participants