Skip to content

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

Merged
mergify[bot] merged 1 commit into
haskell:masterfrom
Bodigrim:no-T.readFile
Jan 25, 2022
Merged

Use T.decodeUtf8 + BS.readFile instead of T.readFile#2637
mergify[bot] merged 1 commit into
haskell:masterfrom
Bodigrim:no-T.readFile

Conversation

@Bodigrim

@Bodigrim Bodigrim commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

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.

@pepeiborra pepeiborra left a comment

Copy link
Copy Markdown
Collaborator

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

jneira commented Jan 25, 2022

Copy link
Copy Markdown
Member

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

jneira commented Jan 25, 2022

Copy link
Copy Markdown
Member

Opened #2641

@jneira jneira left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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