relvacode / iso8601

A fast ISO8601 date parser for Go
MIT License
144 stars 16 forks source link

Various v2 additions #10

Closed rickb777 closed 1 year ago

rickb777 commented 3 years ago
  1. Fixes error in go.mod - this is necessary for anyone to use v2.x
  2. Adds Of function for convenient (i.e less syntax) conversion from time.Time to iso8601.Time.
  3. Adds Equal and String methods to override the base time.Time behaviour (which is inappropriate)
  4. Adds more test cases
  5. Adds some comments to explain some reasoning

This is a bit of a smorgasbord with 5 minor things in one PR. I hope you think it's a worthwhile step forward though. We can discuss merging the different parts separately if you feel it's necessary.

relvacode commented 3 years ago

I like most of this, and thanks for working on it.

I have some reservations about the Equal override though, by changing the signature of Equal we're also making the iso8601 time type incompatible with the public interface of time.Time.

One could imagine this implementation

// satisfied for both time.Time and iso8601.Time
type TimeEqualler interface {
    Equal(time.Time) bool
}

func CheckIfEqual(t TimeEqualler) {
    t.Equal(internalTime)
}

If the other way is needed, .Time can still be used to get at the standard lib time without needing to break interface parity. Plus to me it doesn't make sense to do it only for Equal when methods like After exists as well.

rickb777 commented 3 years ago

I don't think the (current) embedded Equal method will work correctly.

Take a look at https://play.golang.org/p/N7s_HoBc5ce, which does not compile although it looks like it ought to. The receiver and parameter are both of type iso8601.Time but the embedded Time.Equal method expects time.Time as its argument.

That was my reason for including a method that overrides the embedded one: the new method does the obvious thing of allowing two values of iso8601.Time to be compared.

I don't have a strong view on this. If you prefer not to include it I am happy to delete it from the PR. I understand your point on the other methods After etc. We would have to do this consistently or not at all.

rickb777 commented 3 years ago

That probably means the Equal should be split into a separate PR.

relvacode commented 3 years ago

Yep I appreciate that sentiment. I can totally see the case if you're working entirely within the iso8601.Time type, then it would make sense that all methods work off that same type.

It's a shame the time.Time interface doesn't follow Go's own mantra of "accept interfaces, return types" as this is exactly the sort of thing that statement has in mind.

I think we'll exclude it for now and perhaps discuss it in a separate issue. For now could I ask one more thing that you update the README example to use the v2 import so it's consistent with the go.mod?

rickb777 commented 3 years ago

I edited the release notes a little - no implied criticism, I was working on the basis that two minds do a better job than one.

rickb777 commented 3 years ago

I'm wondering about the decision not to support 0000-00-00T00:00:00. If it might be received in JSON, the behaviour I would expect would be to parse it cleanly as zero time.Time{} instead of giving an error. What are your thoughts?

relvacode commented 1 year ago

I'm going to close this PR due to age. Any specific changes please raise as separate issues