adrianmo / go-nmea

A NMEA parser library in pure Go
MIT License
226 stars 77 forks source link

DateTime function to convert to time.Time value (close #115) #116

Closed mholt closed 1 month ago

mholt commented 1 month ago

This change adds a DateTime function as discussed in #115.

icholy commented 1 month ago

@aldas are you ok with this API?

mholt commented 1 month ago

That's a valid point; I guess it's more conventional to have optional arguments last. Refactored and pushed :)

icholy commented 1 month ago

They sorry for the delay, I haven't had access to my computer for the past couple days.

should referenceYear be first parameter as it is optional and d and t are "main" things here we are combining but they come after "optional" parameter.

The referenceYear parameter isn't optional though, it must be explicitly passed every time. If Go did support optional parameters then putting it last would make sense because you'd be able to omit it at the call site.

nmea.DateTime(d, t)

I put the referenceYear first because it's the most significant unit. It's roughly modeled after time.Date signature which follows the same left to right pattern.

func Date(year int, month Month, day, hour, min, sec, nsec int, loc *Location) Time

I won't die on this hill though. @mholt care to be the tie breaker on this?

mholt commented 1 month ago

I guess we technically could do func DateTime(d Date, t Time, referenceYear ...int) time.Time to make it optional.

But I think your point about "most significant unit" is a good one, and I like the parity with time.Date. Century, year, time. Or put another way: First two year digits, last 2 year digits and rest of date, time.

It's also probably best for this value to be explicitly set. Psychologically speaking, having it be the first argument may be a good way to encourage that.

I just switched it back.

mholt commented 1 month ago

Thank you both!!