Fix for YAML spec failure with new nanoseconds precision

[straight-shoota]

This fixes a failing spec which was introduced by merging #5007 and #5022

/cc @asterite @mverzilli

[mverzilli]

Aaaaand with this change you've unblocked Crystal's 0.24 release 🎉

[RX14]

I think that the breaking change in the time constructor introduced by #5022 this PR highlights was unneccesary. It's silently broken this in a very subtle way, I think breaking changes should be a lot less subtle and easy to miss than this.

[straight-shoota]

Yes, I was actually surprised that #5022 didn't break much at all.

[RX14]

I'd rather #5022 was changed. Or at least more discussion of that.

[bew]

I think that the breaking change in the time constructor introduced by #5022 this PR highlights was unneccesary. It's silently broken this in a very subtle way, I think breaking changes should be a lot less subtle and easy to miss than this.

I agree, maybe we should leave the constructor with the sane logic it had before, and add (one more?) Time constructor with a named argument to specify even more precision?

[mverzilli]

I don't think it's a good idea to keep adding constructors just for backwards compatibility pre-1.0, although I agree that the situation is a bit confusing. I think the root cause here is that Time constructors are accepting a lot of unnamed primitive type params. Maybe we should avoid introducing constructors like that into the stdlib in the future.

[akzhan]

I'm sorry but pull requests broke a lot a things :) Now proposed #5070 to eliminate bugs found.

It's strange for me that #5069 passes the specs :)

[straight-shoota]

In the long run, it makes more sense to have the default argument after seconds be nanoseconds because that's the precision we want. Because currently it's milliseconds, it needs to break at some point.

It might be the right thing to have it break now. It might also be better to wait for upcoming changes to time class and break multiple things at once instead of little steps.

An idea for a smooth transition could be to use the argument as nanosecond but only allow millisecond (or microsecond) precision for now. Until the next release, all values smaller than 1000 could throw an exception to allow identifying millisecond values. Afterwards the limit could be removed to allow full nanosecond precision. If nanosecond precision is needed right away, the new constructor with second and nanosecond named arguments can be used.

[akzhan]

It looks like a part of fix (and specs are test Time values incorrectly, I suppose, subject for another investigation). So please review #5070 instead of this PR.

[akzhan]

Formatters and parsers are broken AFAIS, and I have proposed #5070. I don't know why you code pass the specs :)

[asterite]

They are not broken. They just lack precision, but that's expected because at that time Time didn't have that precision... So #5070 is a feature, not a bug fix.

[asterite]

I'm fine with having different overloads that accept millisecond and nanosecond as required named arguments to avoid confusion. So if someone wants to implement that, go ahead.

[asterite]

@RX14 I think this can be merged. I think the changes you are requesting are "you should have fixed this in a PR that was already merged", but that's a bit impossible right now.

[mverzilli]

Let's start by merging this so we get a passing master again.