exprotobuf icon indicating copy to clipboard operation
exprotobuf copied to clipboard

Support for :mapfields_as_maps

Open tiagopog opened this issue 8 years ago • 7 comments

Hey there!

Recently I've opened an issue on grpc-elixir trying to figure out a way to parse proto's maps as Elixir's maps rather than a list of tuples (default).

Taking a look at exprotobuf and gpb, I saw that the second accepts an option for such kind of map parsing. Then I tried to pass both options [:mapfields_as_maps] and [{:maps, :mapfields_as_maps}] to the parse functions but it didn't work as expected.

I couldn't find any test/spec covering this case so I'm just wondering how should I proceed to make use of this option and be able to use Elixir maps on my map fields.

tiagopog avatar Feb 12 '17 02:02 tiagopog

I'll take a look at this, I wonder if that's a more recent option, as I don't recall it being present before. We definitely want to support it!

bitwalker avatar Feb 14 '17 17:02 bitwalker

So looking at the code, options provided via the exprotobuf API are never passed to :gpb, they are set automatically based on context. I'll look into making those options default, and see if that causes any backwards compatibility issues - if not, we're good, if they do, then I'll need to provide a way to expose it as an option and do a deprecation cycle.

bitwalker avatar Feb 14 '17 18:02 bitwalker

Sounds legit to me, @bitwalker 👍

tiagopog avatar Feb 14 '17 19:02 tiagopog

@bitwalker I'm very interested in this feature as well. Any further news on it? Would it be alright if I workeh on it, and if so, any tips on where to get started?

DavidMikeSimon avatar Dec 17 '17 04:12 DavidMikeSimon

@DavidMikeSimon there's a fork with this feature already working (see #73). The thing is I got very busy with other open|closed source projects and I haven't had enough free time to finish this one, but I'm still planning to do so.

tiagopog avatar Dec 17 '17 14:12 tiagopog

Uh, and if you need any guidance for testing this feature from the fork, please let me know.

tiagopog avatar Dec 17 '17 14:12 tiagopog

@tiagopog Thank you! I've been playing around a little with your fork, and I am having some difficulties. The unit tests are failing for me, and it seems to be because the new Protobuf.compile function is being given quoted expressions instead of their actual values. For example, the call to use in map_test.exs looks like this:

  defmodule Msgs do
    use Protobuf, from: Path.expand("../proto/map.proto", __DIR__)
  end

Which should work, but it results in this compilation error:

--- ~/exprotobuf ‹feature/use-gpb-compile* M› » mix test test/protobuf/map_test.exs

[from: {{:., [line: 5],
   [{:__aliases__, [counter: 0, line: 5], [:Path]}, :expand]}, [line: 5],
  ["../proto/map.proto", {:__DIR__, [line: 5], nil}]}]
** (FunctionClauseError) no function clause matching in Protobuf.compile/3

    The following arguments were given to Protobuf.compile/3:

        # 1
        :proto

        # 2
        {{:., [line: 5], [{:__aliases__, [counter: 0, line: 5], [:Path]}, :expand]}, [line: 5], ["../proto/map.proto", {:__DIR__, [line: 5], nil}]}

        # 3
        nil

    Attempted function clauses (showing 2 out of 2):

        def compile(:proto, path, options) when is_binary(path)
        def compile(:proto, paths, options) when is_map(paths)

    (exprotobuf) lib/exprotobuf.ex:69: Protobuf.compile/3
    (exprotobuf) expanding macro: Protobuf.__using__/1
    test/protobuf/map_test.exs:5: Protobuf.Map.Test.Msgs (module)
    (elixir) expanding macro: Kernel.use/2
    test/protobuf/map_test.exs:5: Protobuf.Map.Test.Msgs (module)
    (elixir) lib/code.ex:376: Code.require_file/2
    (elixir) lib/kernel/parallel_require.ex:59: anonymous fn/2 in Kernel.ParallelRequire.spawn_requires/5

To fix this, it seems like I'd end up having to move the compile calls into Builder.define, where I can unquote the options. Am I misunderstanding something here? I'm new to Elixir, so it's likely that I'm overlooking something obvious. :-)

DavidMikeSimon avatar Dec 18 '17 02:12 DavidMikeSimon