Elixir terms converter extensions

[ulissesalmeida]

Hi 👋

I would like to know if the maintainers are interested in a feature like this.

Let's say I have this message:

package MyApp.Proto;

message NaiveDateTime {
  uint32 microseconds = 1;
}

Then, when I working I'm my app, I've always to convert to an Elixir term.

message.birthday
%MyApp.Proto.NaiveDateTime{microseconds: ...}

# oh no, I have to remember to:
to_naive_date_time(message.birthday)
~N[1988-10-29 00:00:00]

I was wondering, wouldn't it be nice to have a way to tell the encoder/decoder how they work with some messages. Then, I could write in my app:

message.birthday
~N[2000-01-01 00:00:00]

I really would like to help to add this feature, but I would like to hear from you folks if you are interested.

Then, we can discuss the API and how this extension should work.

I had a suggestion, we could have something like:

message NaiveDateTime {
  option (elixirpb.message).converter = "MyApp.NaiveDateTimeConverter";
  uint32 microseconds = 1;
}

Then in my app, I implement some module behavior:

defmodule MyApp.NaiveDateTimeConverter do
  @behaviour Protobuf.Converter

  # or post_decode, decode, after_decode
  def to_elixir(%MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}),
    do: NaiveDateTime.from_unix!(microseconds, :microsecond)

  # or pre_encode, encode, before_encode
  def to_protobuf(%DateTime{} = datetime) do
    microseconds = DateTime.to_unix(datetime, :microsecond)
    %MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}
  end

  def to_protobuf(%NaiveDateTime{} = naive) do
    {:ok, datetime} = DateTime.from_naive(naive, "Etc/UTC")
    microseconds = DateTime.to_unix(datetime, :microsecond)
    %MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}
  end

  def elixir_type, do: "NaiveDateTime.t() | DateTime.t()"
end

What do you folks think? Do we have space in this library for such a feature?

[ulissesalmeida]

The other idea for API is would be using protocols.
Something like:

message NaiveDateTime {
  option (elixirpb.message).type_name = "NaiveDateTime";
  uint32 microseconds = 1;
}

Then in my app, I implement protocols like:

defimpl Protobuf.Decodable, for: MyApp.Protobuff.NaiveDateTime do
  # or post_decode, decode, after_decode
  def to_elixir(%MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}),
    do: NaiveDateTime.from_unix!(microseconds, :microsecond)
end

defimpl Protobuf.Encodable, for:  DateTime do
  # or pre_encode, encode, before_encode
  def to_protobuf(%DateTime{} = datetime, MyApp.Protobuff.NaiveDateTime) do
    microseconds = DateTime.to_unix(datetime, :microsecond)
    %MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}
  end
end

defimpl Protobuf.Encodable, for: NaiveDateTime  do
   # or pre_encode, encode, before_encode
  def to_protobuf(%NaiveDateTime{} = naive, MyApp.Protobuff.NaiveDateTime) do
    {:ok, datetime} = DateTime.from_naive(naive, "Etc/UTC")
    microseconds = DateTime.to_unix(datetime, :microsecond)
    %MyApp.Protobuff.NaiveDateTime{microseconds: microseconds}
  end
end

[darrenclark]

I like the protocols approach. #90 would be possible with this approach too

Then https://github.com/tony612/google-protos could be updated to handle the conversions to/from Well-Known types (includes the wrapper types)

[ulissesalmeida]

@darrenclark I agree with you that it's possible to achieve the ValueWrappers feature by using these protocols.

The downside is the work and the task to remember to implement the protocol.

In our application, we have dozens of enum and wrappers, whenever a developer adds a new enum they would have to remember to implement the protocol. It is not that bad, but it would be very convenient to have this behaviour automatically without adding any new code for every new wrapper. But of course, you would have to opt-in, by default is disabled.

[ulissesalmeida]

I finished the workload of the protocols here + type annotation here: #91

Anytime you folks have time review it or give a try
cc @darrenclark @tony612

[darrenclark]

Re: #91, nice!

Haven't had a chance to play with it, but looks like this would help in my project quite a bit. Especially for converting to/from DateTime's

[whatyouhide]

@ulissesalmeida on principle, I’m somewhat okay with this feature. However, in the current state of the library with all the issues (a bunch of which are bugs) and PRs that are open, I think it would be beneficial to focus on getting this back into a state of excellence with a smaller feature set. Once we're there, I'd be happy to revisit this. For now, this would solve an inconvenience and not an actual problem so I'd say we close this and reopen it in the future. Thoughts?

[ulissesalmeida]

@whatyouhide Coingaming uses heavily this feature, without it, they will not be able to migrate from their fork to the main repo.
Unless they do a huge workforce to stop using this convenience. Maybe @thalesflores have some inputs.

I personally don't feel attached because I'm not working there anymore 🤣
So, I'm 👍🏽 for what is the best for the library. I don't think the library needs to tackle all the peculiarities of a single company. However, that convenient idea comes from other protobuf libraries in other languages.

• Ruby: Add wrapper type helpers for Ruby protocolbuffers/protobuf#5739
• Typescript: https://github.com/stephenh/ts-proto#wrapper-types

I still think it would be nice to have.

[whatyouhide]

I don't think the library needs to tackle all the peculiarities of a single company

Yep, this is my main point too. They can still keep a fork of this up to date with features on top of it, but unless we get additional maintainers here, I’m a bit wary of adding new features :) Thanks anyways!