Shall not include Time conversion by default

[vladfaust]

It's even stated in the docs:

https://github.com/crystal-community/msgpack-crystal/blob/4c123dfe9ba76a24c2b09e442e66e9df37406fe3/src/message_pack/from_msgpack.cr#L202-L204

I suggest that proper implementation does not "assume" anything. There should be a converter for anything which is out of MessagePack specification scope.

A Time can be transferred either as ISO 8601 string or epoch seconds or even epoch milliseconds. There should not be a default constructor for Time IMO.

[kostya]

doc not updated, why not, i think its ok, if you want special format you set it.

[vladfaust]

It's bad because your're defining the single possible variant for Time parsing from msgpack, and this should not be a case. It's better to have something like Time::StringConverter or so.

[kostya]

this not single possible variant, you can do many things:

require "./src/msgpack"

msg = Time.now.to_msgpack(Time::Format.new("%F"))
p msg
p String.from_msgpack(msg)
time = Time.from_msgpack(Time::Format.new("%F"), msg)
p time

class A
  include MessagePack::Serializable

  @[MessagePack::Field(converter: Time::Format.new("%F %T"))]
  property value : Time?
end

a = A.new
a.value = Time.now

msg = a.to_msgpack
p msg
p A.from_msgpack(msg)

Bytes[170, 50, 48, 49, 57, 45, 48, 53, 45, 48, 49]
"2019-05-01"
2019-05-01 00:00:00.0 UTC
Bytes[129, 165, 118, 97, 108, 117, 101, 179, 50, 48, 49, 57, 45, 48, 53, 45, 48, 49, 32, 49, 51, 58, 52, 57, 58, 52, 55]
#<A:0x1006cdea0 @value=2019-05-01 13:49:47.0 UTC>

maybe string conversion by default not optimal, but default case just simplify things.

[vladfaust]

What if me or a third party wants to store time in Unix seconds?

[kostya]

struct UnixConverter
  def self.from_msgpack(unpacker)
    Time.unix_ms(unpacker.read_int)
  end

  def self.to_msgpack(value, packer)
    packer.write(value.to_unix_ms)
  end
end

msg = Time.now.to_unix_ms.to_msgpack
p msg
p Int64.from_msgpack(msg)
p UnixConverter.from_msgpack(MessagePack::IOUnpacker.new(msg))

class B
  include MessagePack::Serializable

  @[MessagePack::Field(converter: UnixConverter)]
  property value : Time?
end

a = B.new
a.value = Time.now

msg = a.to_msgpack
p msg
p B.from_msgpack(msg)

Bytes[207, 0, 0, 1, 106, 115, 26, 101, 110]
1556709270894
2019-05-01 11:14:30.894000000 UTC
Bytes[129, 165, 118, 97, 108, 117, 101, 207, 0, 0, 1, 106, 115, 26, 101, 111]
#<B:0x10d631ea0 @value=2019-05-01 11:14:30.895000000 UTC>

[vladfaust]

Yes, so why do we use a converter for unix time but don't use one for ISO formatted time? Why does ISO format has higher "precedence" in this implementation? That's what I'm talking about.

[kostya]

may be unix by default would be better, i just choose the same as json converter.

[vladfaust]

There should not be any defaults IMO unless stated otherwise by MessagePack specs. It is a breaking change indeed, but it would reduce possible confusion. I guess I have to also create an issue in Crystal itself, but afraid that the core team would deny it due to "it works as it works" and "it's too breaking" and "you're bikeshedding". We'll see.

Regarding to this repo, I could create a PR creating TimeFormat and also Epoch (Unix?) converters.

[kostya]

i not sure that default need to be deleted, at least you can redefine default also:

require "msgpack"

struct Time
  def to_msgpack(packer : MessagePack::Packer)
    packer.write(self.to_unix)
  end

  def self.new(pull : MessagePack::Unpacker)
    Time.unix(pull.read_int)
  end
end

msg = Time.now.to_msgpack
p msg
p Time.from_msgpack(msg)

[didactic-drunk]

I have to agree with no default or a standard extension default. The current default wasted a bunch of time debugging an issue with sub seconds. The current default doesn't store them. Crystal to_s doesn't show them. What seems like two identical Time's compare as false most of the time. I think that should be a bug.

Example:

t1 = Time.now
t2 = Time.from_msgpack t1.to_msgpack

# Because of nanoseconds comparisons can be true or false for the same encoded time value.
t1 == t2 => false # Most of the time.
t1 == t2 => true # 1 in a million.

# Visually looks identical and like == is broken
p t1, t2

[didactic-drunk]

A Time can be transferred either as ISO 8601 string or epoch seconds or even epoch milliseconds. There should not be a default constructor for Time IMO.

I've also seen Floats used. And "#{epoch}.#{milliseconds}" And non ISO time String's. I wouldn't be surprised if someone used TAI in a number format that's easily confused with various epoch encodings.