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

read json file with utf-8 #550

Closed
wants to merge 3 commits into from
Closed

Conversation

MagicCodess
Copy link

Different language platform especially in windows may cause error here, read json file with utf-8 decoder might work better.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

Hi @MagicCodess 👋 I think a more robust solution to this problem for you is to instead set your locale preferred encoding to UTF-8.

Making this change may resolve a particular error for you, but it won't necessarily resolve all such errors when using this library or upstream libraries like FiftyOne. Also, this library doesn't necessarily have a UTF-8 only assumption, so I'd need to consider this a bit more globally to see what the broader implications might be to make a proper promise on string encoding support.

@MagicCodess
Copy link
Author

hi @brimoor :), I think the graceful way for this is to detect the json file encoding, in this way users don't need to change their locale encoding which may cause some errors in their other apps. I have committed some new codes which using chardet to detect file encoding, thanks for your reviewing.

Copy link
Contributor

@brimoor brimoor left a comment

Choose a reason for hiding this comment

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

Hmm while this is fancy, in the worst case it could make loading JSON 2x slower...

I think I'm inclined to go with your UTF-8-only assumption instead for simplicity. But, again, we'll need to check for more places where this assumption may need to be implemented. For example write_json() in this library. But also in FiftyOne which is the primary dependency of this library.

@brimoor brimoor closed this May 31, 2022
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.

None yet

2 participants