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

Use T.decodeUtf8 + BS.readFile instead of T.readFile #2637

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Use T.decodeUtf8 + BS.readFile instead of T.readFile #2637

merged 1 commit into from
Jan 25, 2022

Conversation

Bodigrim
Copy link
Contributor

@Bodigrim Bodigrim commented Jan 24, 2022

As documentation (https://hackage.haskell.org/package/text-2.0/docs/Data-Text-IO.html#v:readFile) explains, T.readFile is almost always a mistake. It's highly unlikely that files we try to read with T.readFile are in a locale encoding and not in UTF-8. Moreover, with text-2.0 fmap T.decodeUtf8 . BS.readFile is up to 10x faster than T.readFile.

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Sounds like we need to deprecate T.readFile and introduce T.readFileUtf8 ?

@jneira
Copy link
Member

jneira commented Jan 25, 2022

It's highly unlikely that files we try to read with T.readFile are in a locale encoding and not in UTF-8

But it is more than possible that some users, specially in windows are using a local encoding, even without being aware of it.
I guess this pr would break hls for them
so imo at least a warning should be added in the changelog (will do myself)
Ideally a LSP alert warning should be added when loading a file if we detect its encoding is not utf8, will open an issue about

@jneira
Copy link
Member

jneira commented Jan 25, 2022

Opened #2641

Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@jneira jneira added the merge me Label to trigger pull request merge label Jan 25, 2022
@Bodigrim
Copy link
Contributor Author

But it is more than possible that some users, specially in windows are using a local encoding, even without being aware of it.
I guess this pr would break hls for them

No, it's actually vice versa. This PR fixes HLS for people, whose locale is not UTF-8. GHC mandates that source files are in UTF-8 only, but before this PR HLS would try to use a locale encoding instead.

@mergify mergify bot merged commit 167e556 into haskell:master Jan 25, 2022
@Bodigrim Bodigrim deleted the no-T.readFile branch January 25, 2022 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants