Introduce :trim_bom option for File.stream!/2 #5702

AndrewDryga · 2017-01-25T19:56:21Z

Relates to #5695.

josevalim · 2017-01-25T20:00:41Z

Thanks @AndrewDryga. I would still like a discussion if this should be opt-in or not. I believe it should because otherwise File.read! will be different from File.stream!. UNLESS File.read!("path/to/file", [:utf8]) already strips bom... but i don't think that's the case?

AndrewDryga · 2017-01-25T20:03:55Z

@josevalim there are no File.read/2. Docs.

josevalim · 2017-01-25T20:10:33Z

@AndrewDryga and i assume we don't strip it even on File.read!/1, right?

josevalim · 2017-01-25T20:11:14Z

@AndrewDryga it seems from your comment though that we always strip it?

AndrewDryga · 2017-01-25T20:51:28Z

@josevalim Looks like we don't, this test passes:

    test "read with UTF-8 BOM" do
      assert {:ok, <<239, 187, 191>> <> "Русский\n日\n"} = File.read(Path.expand('fixtures/utf8_bom.txt', __DIR__))
    end

I guess your were right that behaviors of File.stream/1 and File.read/1 don't match, since first one will skip BOM's, and second one don't. I would appreciate any thoughts how core-team want to see it fixed.

josevalim · 2017-01-25T20:57:44Z

@AndrewDryga I believe the next step is to find out where stream is stripping out the bom, so we can consider the available options.

josevalim · 2017-01-25T20:57:54Z

It is probably something related to IO, such as IO.read.

AndrewDryga · 2017-01-25T21:13:35Z

@josevalim looks like it's in file.ex. File.stream/1 uses functions to prepare modes, and they adding :utf8 to modes (and File.read doesn't call this funcs):

  defp normalize_modes([:utf8 | rest], binary?) do
    [encoding: :utf8] ++ normalize_modes(rest, binary?)
  end
  defp normalize_modes([:read_ahead | rest], binary?) do
    [read_ahead: @read_ahead_size] ++ normalize_modes(rest, binary?)
  end
  # TODO: Remove :char_list mode by 2.0
  defp normalize_modes([mode | rest], _binary?) when mode in [:charlist, :char_list] do
    if mode == :char_list do
      IO.warn "the :char_list mode is deprecated, use :charlist"
    end
    normalize_modes(rest, false)
  end
  defp normalize_modes([mode | rest], binary?) do
    [mode | normalize_modes(rest, binary?)]
  end
  defp normalize_modes([], true), do: [:binary]
  defp normalize_modes([], false), do: []

AndrewDryga · 2017-01-25T21:14:05Z

  def stream!(path, modes \\ [], line_or_bytes \\ :line) do
    modes = normalize_modes(modes, true)
    File.Stream.__build__(IO.chardata_to_string(path), modes, line_or_bytes)
  end

vs

  def read(path) do
    F.read_file(IO.chardata_to_string(path))
  end

AndrewDryga · 2017-01-25T21:16:53Z

Or maybe not, i'll investigate further.

josevalim · 2017-01-25T21:17:02Z

@AndrewDryga FIle.stream! does not use utf8 by default though, does it?

AndrewDryga · 2017-01-25T23:19:07Z

@josevalim sorry, spent a while to figure this out.

My test was initially wrong, because of copy-pasting text from source I've managed to put BOM into test assertion. Both function (stream!/1 and read!/1) are returning strings with BOM.

AndrewDryga · 2017-01-27T20:25:49Z

@josevalim I guess it would be better to implement :strip_bom modifier, so we won't break backwards compatibility. Any suggestions on it's implementation? At the moment I can see two ways:

Pattern match BOM's in reduction function of Enumerable implementation.
Use {encoding, utf8} while opening file and then read lines with something familiar to io:get_line(Pid, x). Possible to put it instead of IO.each_binstream/2 usage in :lines mode.

josevalim · 2017-01-27T20:39:19Z

I would prefer 1. but ideally we want to do it only on the first line. How is UTF-8 encoding supposed to behave in face of multiple BOMs?

AndrewDryga · 2017-01-27T20:43:01Z

@josevalim There are should not be multiple BOM's (second one should be part of binary document), so we need to remove it only once. However there are many different kinds of BOM's for different types of UTF encodings.

We can add line counter/boolean flag to acc and remove them only once.

AndrewDryga · 2017-01-28T18:10:19Z

I guess test needs to be restarted, since it looks like a random VM issue.

AndrewDryga · 2017-01-28T20:21:22Z

lib/elixir/lib/file/stream.ex

@@ -73,7 +73,8 @@ defmodule File.Stream do
      start_fun =
        fn ->
          case :file.open(path, read_modes(modes)) do
-            {:ok, device}    -> device
+            {:ok, device}    ->
+              if :strip_bom in modes, do: strip_bom(device), else: device


This could be done better, I don't like nesting conditions. Should I match option value as second argument in strip_boom fn?

Small possible refactoring:

strip_bom? = :strip_bom in modes case :file.open(path, read_modes(modes)) do {:ok, device} when strip_bom? -> strip_bom(device) {:ok, device} -> device {:error, reason} -> ... end

Maybe better, but maybe not ¯\_(ツ)_/¯

I think this is counter-intuitive usage of guard. What do you think about something like this:

case :file.open(path, read_modes(modes)) do {:ok, device} -> maybe_strip_boom(device) {:error, reason} -> ... end

and

def maybe_strip_boom(device, modes) when :strip_bom in modes, do: .. def maybe_strip_boom(device, _modes), do: device

The current code is fine. Unfortunately we cannot write the proposed approach because that's not an valid guard. :(

I don't like this solution. You don't need to know the logic of whether to stripping BOM in a function whose purpose is to strip the BOM, IMO. This usage of guards is not rare in Elixir though :) But if you don't want to go with when strip_bom?, I think the if solution looks alright.

Is there are any best practice on "when we should split function" in Elixir source? Level of nesting, ABC complexity, etc..?

@josevalim

iex> strip_bom? = true iex> case {:ok, 1} do ...> {:ok, _} when not strip_bom? -> :a ...> {:ok, _} -> :b ...> end :b

🤔

whatyouhide · 2017-01-28T21:23:18Z

lib/elixir/lib/file/stream.ex

@@ -73,7 +73,8 @@ defmodule File.Stream do
      start_fun =
        fn ->
          case :file.open(path, read_modes(modes)) do
-            {:ok, device}    -> device
+            {:ok, device}    ->
+              if :strip_bom in modes, do: strip_bom(device), else: device


Small possible refactoring:

strip_bom? = :strip_bom in modes case :file.open(path, read_modes(modes)) do {:ok, device} when strip_bom? -> strip_bom(device) {:ok, device} -> device {:error, reason} -> ... end

Maybe better, but maybe not ¯\_(ツ)_/¯

whatyouhide · 2017-01-28T21:25:33Z

lib/elixir/lib/file/stream.ex

    defp read_modes(modes) do
-      for mode <- modes, mode not in [:write, :append], do: mode
+      for mode <- modes, mode not in [:write, :append, :strip_bom], do: mode


Since we're touching this, would Enum.filter/2 be a better fit here? 🤔

Enum.filter(modes, &(&1 not in [:write, :append, :strip_bom]))

I would personally prefer Enum.uniq(modes) -- [:write, :append, :strip_bom]

A version with just -- would be ideal, but there's a possibility of duplicate entries.

I prefer the current version. ;)

josevalim · 2017-01-28T21:33:45Z

lib/elixir/lib/file.ex

@@ -1258,6 +1258,11 @@ defmodule File do
  One may also consider passing the `:delayed_write` option if the stream
  is meant to be written to under a tight loop.

+  ## Byte order marks
+
+  If you pass `:skip_bom` in the modes parameter, the stream will


:strip_bom. :)

I'm wondering, maybe we should use trim_bom instead? This prefix we use in the String module.

Agreed with @lexmag, it feels slightly inconsistent now that we removed all the "strip" stuff in String 😕

Yes, :trim_bom. @AndrewDryga can you please amend the PR accordingly? Sorry for the late minute change. :)

Sure, I'll change it.

josevalim · 2017-01-28T21:35:24Z

lib/elixir/lib/file.ex

@@ -1388,4 +1393,18 @@ defmodule File do
    do: path
  defp maybe_to_string(path),
    do: IO.chardata_to_string(path)
+
+  @doc false
+  def bom_length(<<239, 187, 191, _rest::binary>>),


If we are not using this function here, we can move it to the File.Stream module.

I decided to place it here, because there may be other places that may use this function, for example some IO/File.read fun may also benefit from :trim_bom mode. But thats my bad, "premature refactoring".. :)

josevalim · 2017-01-28T21:36:00Z

lib/elixir/lib/file/stream.ex

@@ -73,7 +73,8 @@ defmodule File.Stream do
      start_fun =
        fn ->
          case :file.open(path, read_modes(modes)) do
-            {:ok, device}    -> device
+            {:ok, device}    ->


Since you are changing this line, can you please remove the indentation of the ->? We no longer do such alignments in Elixir.

I'm totally +1 for that.

josevalim · 2017-01-28T21:37:36Z

I have added some final comments and this should be good to go!

lexmag · 2017-01-28T21:42:04Z

lib/elixir/test/elixir/file_test.exs

@@ -1322,6 +1322,26 @@ defmodule FileTest do
    assert Enum.count(stream) == 2
  end

+  test "stream keeps BOM" do
+    src = fixture_path("utf8_bom.txt")
+    bom_line = src


We always move initial argument to the next line:

bom_line = src |> File.stream!() |> Enum.take(1) |> List.first()

lexmag · 2017-01-28T21:42:55Z

lib/elixir/test/elixir/file_test.exs

+    |> Enum.take(1)
+    |> List.first()
+
+    assert <<239, 187, 191>> <> "Русский\n" = bom_line


I think we can use == operator in assertion, it gives better message on failure than with =.

Same works for another test.

whatyouhide · 2017-01-28T21:46:21Z

lib/elixir/test/elixir/file_test.exs

+    bom_line = src
+    |> File.stream!()
+    |> Enum.take(1)
+    |> List.first()


Since we know the shape of this, I think the right way to do this is either use hd/1 in place of List.first/1, or even do

[bom_line] = src |> File.stream!() |> Enum.take(1)

Wdyt?

👍 for [bom_line] =.

+1 for [bom_line]. I'd rather wrap something with list, than argue how to extract value from it :).

@whatyouhide why hd/1 is better than List functions?

AndrewDryga · 2017-01-29T15:54:57Z

@josevalim everything is fixed 👍

josevalim · 2017-01-29T18:13:33Z

❤️ 💚 💙 💛 💜

Added test for BOM stripping when using File.stream!/1

36236c0

AndrewDryga added 2 commits January 26, 2017 01:19

Removed BOM from test

0f6a770

Added obvious BOM to test so it will pass

3f1b0d7

Move file pointer to skip BOM

9371bce

AndrewDryga changed the title ~~[WIP] Do not deliver BOM when using File.stream/1~~ Introduce :strip_bom option for File.stream!/2 Jan 28, 2017

AndrewDryga added 2 commits January 28, 2017 19:38

Typo

4e63aad

Added docs

7591aa4

AndrewDryga commented Jan 28, 2017

View reviewed changes

whatyouhide reviewed Jan 28, 2017

View reviewed changes

josevalim reviewed Jan 28, 2017

View reviewed changes

lexmag suggested changes Jan 28, 2017

View reviewed changes

whatyouhide reviewed Jan 28, 2017

View reviewed changes

Minor changes based on GitHub comments

6961437

lexmag approved these changes Jan 28, 2017

View reviewed changes

josevalim changed the title ~~Introduce :strip_bom option for File.stream!/2~~ Introduce :trim_bom option for File.stream!/2 Jan 29, 2017

josevalim approved these changes Jan 29, 2017

View reviewed changes

josevalim merged commit 8a8e367 into elixir-lang:master Jan 29, 2017

Introduce :trim_bom option for File.stream!/2 #5702

Introduce :trim_bom option for File.stream!/2 #5702

Conversation

AndrewDryga commented Jan 25, 2017

josevalim commented Jan 25, 2017

AndrewDryga commented Jan 25, 2017

josevalim commented Jan 25, 2017

josevalim commented Jan 25, 2017

AndrewDryga commented Jan 25, 2017

josevalim commented Jan 25, 2017

josevalim commented Jan 25, 2017

AndrewDryga commented Jan 25, 2017 • edited Loading

AndrewDryga commented Jan 25, 2017

AndrewDryga commented Jan 25, 2017

josevalim commented Jan 25, 2017

AndrewDryga commented Jan 25, 2017

AndrewDryga commented Jan 27, 2017 • edited Loading

josevalim commented Jan 27, 2017 • edited Loading

AndrewDryga commented Jan 27, 2017 • edited Loading

AndrewDryga commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewDryga Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewDryga commented Jan 29, 2017

josevalim commented Jan 29, 2017

AndrewDryga commented Jan 25, 2017 •

edited

Loading

AndrewDryga commented Jan 27, 2017 •

edited

Loading

josevalim commented Jan 27, 2017 •

edited

Loading

AndrewDryga commented Jan 27, 2017 •

edited

Loading

AndrewDryga Jan 28, 2017 •

edited

Loading