bitwalker / exprotobuf

Protocol Buffers in Elixir made easy!
Apache License 2.0
486 stars 69 forks source link

Support for :mapfields_as_maps #61

Open tiagopog opened 7 years ago

tiagopog commented 7 years ago

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.

bitwalker commented 7 years ago

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 commented 7 years ago

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.

tiagopog commented 7 years ago

Sounds legit to me, @bitwalker 👍

DavidMikeSimon commented 6 years ago

@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?

tiagopog commented 6 years ago

@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 commented 6 years ago

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

DavidMikeSimon commented 6 years ago

@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. :-)