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)
On Monday, July 18, 2022 at 7:04:51 AM UTC José Valim wrote:
> The PR does not attempt to do a conversion back to known cases, so adding
> such feature is still welcome!
>
> On Mon, Jul 18, 2022 at 9:02 AM José Valim <[email protected]> wrote:
>
>> Actually, a PR was already sent here:
>> https://github.com/elixir-lang/elixir/pull/11991 :)
>>
>>
>> On Mon, Jul 18, 2022 at 8:57 AM José Valim <[email protected]> wrote:
>>
>>> 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!
>>>
>>> On Mon, Jul 18, 2022 at 05:41 Quentin Crain <[email protected]> wrote:
>>>
>>>> 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)
>>>>
>>>> --
>>>> 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 [email protected].
>>>> To view this discussion on the web visit
>>>> https://groups.google.com/d/msgid/elixir-lang-core/30477ff6-7eca-4e66-a118-2bf3920abd34n%40googlegroups.com
>>>>
>>>> <https://groups.google.com/d/msgid/elixir-lang-core/30477ff6-7eca-4e66-a118-2bf3920abd34n%40googlegroups.com?utm_medium=email&utm_source=footer>
>>>> .
>>>>
>>>
--
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 [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/elixir-lang-core/5ff85784-a306-4c8c-879a-e55d62eb4d91n%40googlegroups.com.
