[Proposal] Regex.compile: Document options when options are given as atoms

[Quentin Crain]

Hi!

I hope I am acting appropriately here!

Per this elixirforum post: https://elixirforum.com/t/is-it-normal-that-regex-compile-2-does-not-print-regex-modifiers-when-atoms-are-passed-in-options/48992

When atoms are used as options to Regex.compile, they are not translated into their character equivalents and added to the Regex struct:

iex(6)> Regex.compile("foo", "i")
{:ok, ~r/foo/i}
iex(7)> Regex.compile("foo", [:caseless])
{:ok, ~r/foo/}

Should they be? I have an initial implementation waiting if so . . .

Respectfully,

   Quentin (Q)

[José Valim]

Yes. And if any unknown option is given, perhaps we should store the underlying options in the struct and change the inspect representation to output Regex.compile!(…)

Please open up an issue and PRs welcome too!

--
You received this message because you are subscribed to the Google Groups "elixir-lang-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to elixir-lang-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/30477ff6-7eca-4e66-a118-2bf3920abd34n%40googlegroups.com.

[José Valim]

Actually, a PR was already sent here: https://github.com/elixir-lang/elixir/pull/11991 :)

[José Valim]

The PR does not attempt to do a conversion back to known cases, so adding such feature is still welcome!

[Quentin Crain]

Hi!

I hope to not draw this out .. afaict the goal of the PR is to preserve the method the developer used in providing options. So, I am a bit confused by possibly doing any converting (presumably from options as atoms to their binary char equivalents). This is what I had in mind though and what I implemented locally.

This seems to me to be so by the change in the type of opts field in the structure to OR with a list/list(atom); with the change to how Regexs are inspected being a 2nd evidence.

I have added the following test which seems to show this:

--- a/lib/elixir/test/elixir/inspect_test.exs
+++ b/lib/elixir/test/elixir/inspect_test.exs
@@ -864,6 +864,7 @@ test "regex" do
     assert inspect(~r" \\/ ") == "~r/ \\\\\\/ /"
     assert inspect(~r/hi/, syntax_colors: [regex: :red]) == "\e[31m~r/hi/\e[0m"

+    assert inspect(Regex.compile!("foo", "i")) == ~S'~r/foo/i'
     assert inspect(Regex.compile!("foo", [:caseless])) == ~S'Regex.compile!("foo", [:caseless])'
   end

Finally, I also added some tests for Regex.opts which show options' type are preserved:

   test "opts/1" do
     assert Regex.opts(Regex.compile!("foo", "i")) == "i"
+    assert Regex.opts(Regex.compile!("foo", [:caseless])) == [:caseless]
   end

Though, reviewing the type spec for opts, I believe it should be updated to reflect that options are a binary OR atom list:

-  @spec opts(t) :: String.t()
+  @spec opts(t) :: String.t() | [term]
   def opts(%Regex{opts: opts}) do
     opts
   end

I apologize for the length of this reply but it does seem the present implementation is contra to my thoughts on preferring the binary representation of options over a list.

Respectfully,

   Q (Quentin)

[José Valim]

I agree with your points. A PR with your additional tests and specs will be very welcome! Thank you!

To view this discussion on the web visit https://groups.google.com/d/msgid/elixir-lang-core/5ff85784-a306-4c8c-879a-e55d62eb4d91n%40googlegroups.com.

[Quentin Crain]

Hi!

Done: https://github.com/elixir-lang/elixir/pull/11995

Thx & have a great day!

Q