I agree with your points. A PR with your additional tests and specs will be
very welcome! Thank you!
On Mon, Jul 18, 2022 at 7:25 PM Quentin Crain <czrp...@gmail.com> wrote:
> 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 <jose....@dashbit.co> 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 <jose....@dashbit.co> 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 <czr...@gmail.com> 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 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
>>>>> <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 elixir-lang-core+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/elixir-lang-core/5ff85784-a306-4c8c-879a-e55d62eb4d91n%40googlegroups.com
> <https://groups.google.com/d/msgid/elixir-lang-core/5ff85784-a306-4c8c-879a-e55d62eb4d91n%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 elixir-lang-core+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/elixir-lang-core/CAGnRm4LdEtT%3DLQAxyKktRwh2sqFrObr3mLSn3X8C194GNAjLNw%40mail.gmail.com.
