[Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation

[Vladimir Davydov]

On Wed, Aug 18, 2021 at 04:24:16PM +0300, Safin Timur wrote:
> Thanks, that's very practical approach to collect all complains spread
> across big patchset to the single email - much easier to deal with.
> 
> And just in case, to make sure we are on the same line - could you please
> read the module api documentation I've saved here https://github.com/tarantool/tarantool/discussions/6244#discussioncomment-1043988
> (I keep it more or less updated after all API changes so far)
> 
> 
> On 18.08.2021 11:25, Vladimir Davydov wrote:
> > [ += tarantool-patches - sorry dropped it from Cc by mistake ]
> > 
> > Putting aside minor hitches, like bad indentation here and there,
> > I have some concerns regarding the datetime API:
> > 
> >   - I believe we shouldn't introduce interval in years/months, because
> >     they depend on the date they are applied to. Better add datetime
> >     methods instead - add_years / add_months.
> 
> Either I don't understand your objection, or you missed some the big chunk
> of interval API I described here.

I missed that there's add/sub methods, sorry. (Because they are defined
in datetime:__index, not in the metatable, which is confusing, oh...)

Still, I don't see much point in defining both __add/__sub and add/sub.
If they have different behavior (like it seems they do in your patch
because they do not share implementation), it will be confusing. If
they are equivalent, then why would we need both? We don't have
decimal.add, only __add, for example.

Regarding datetime.years and datetime.months interval constructors,
quoting the RFC you wrote:

	tarantool> T = date('1972-02-29')
	---
	...

	tarantool> M = date.months(2)
	---
	...

	tarantool> Y = date.years(1)
	---
	...

	tarantool> T + M + Y
	---  
	- 1973-04-30T00:00Z
	...

	tarantool> T + Y + M
	---
	- 1973-05-01T00:00Z
	...

	¯\_(ツ)_/¯

But even putting aside different results, I don't understand why
T + M + Y doesn't equal 1973-04-29 and T + Y + M doesn't equal
1973-05-29. Confused. Suggest to drop year and month intervals.
An interval must always be exact.

> 
> The reason why I had to introduce special interval types was this problem of
> adding or subtraction generic interval. It's complicated to calculate number
> of days when you need to subtract or add whole year, it's even more
> complicated becoming when you need to add or subtract several months
> (especially if they are spanning period longer than year). c-dt does have
> special functions for this purpose
> (
> dt_t   dt_add_years    (dt_t dt, int delta, dt_adjust_t adjust);
> dt_t   dt_add_months   (dt_t dt, int delta, dt_adjust_t adjust);
> ) which could reasonably handle all sort of truncations or snapping, if
> month is shorter or longer than original one.
> 
> And having necessity to separate generic interval arithmetic for month, or
> year (or possible quarters) intervals made me to use different cdata type
> for their special storage. We may use any different way to distinguish
> special years, like special field in the generic interval and introduce kind
> of interval as another field. But approach use in SciLua LuaTime
> https://github.com/stepelu/lua-time when we use different cdata type as
> their tag for dispatching, looked most the elegant and easiest one.
> 
> > 
> >   - The API provides too many ways to extract the same information from a
> >     datetime object (m, min, minute all mean the same). IMO a good API
> >     should provide exactly one way to achieve a certain goal.
> 
> Wholeheartedly disagreed. With all due respect. There are many cases when
> it's ok to provide several means for achieaving particular goal. I'm ok in
> such environment with such mind-sets. (That may be explaining why I liked
> C++ and Perl, and disliked Go and Python :))
> 
> I do foresee circumstances wen someone would like to use `date.now() +
> date.weeks(2)`, while others would consistently use `date.now():add{weeks =
> 2}`. Both approaches acceptable, and may be supported equally.

IMO this syntactic sugaring only complicates developers' lives (suspect
they will end up writing guidelines what they can and can't use to keep
the code consistent, like they do in C++).

> 
> > 
> >   - AFAICS datetime.hour returns the number of hours passed since the
> >     year 1970. This is confusing. I don't have any idea why anyone would
> >     need this. As a user I expect them to return the hour of the current
> >     day. Same for other similar methods, like month, minute, year.
> 
> I've introduce minute, hour, and upward as accessors, soon after I've
> provided .nanoseconds, .milliseconds and .seconds accessors.
> 
> Having this discussion here I realize that .hour may be not considered as
> yet another accessor to seconds information, but as hour in a day, this
> datetime is representing.
> 
> I suggest to remove these confusing accessors altogether, and open ticket
> for later additions a method which would return such os.date-like table.
> With attributes properly translated to year/month/day/hour/minute/second.

OK, but we need to agree first on what those accessors will look like:

 - datetime members
 - datetime methods
 - a method that returns a table similar to the one returned by os.date

> 
> > 
> >   - Creating a datetime without a date (with parse_time) looks weird:
> > 
> >     tarantool> dt = datetime.parse_time('10:00:00')
> >     ---
> >     ...
> >     tarantool> tostring(dt)
> >     ---
> >     - 1970-01-01T10:00Z
> >     ...
> > 
> >     Why 1970? I think that a datetime should always be created from a
> >     date and optionally a time. If you want just time, you need another
> >     kind of object - time. After all it's *date*time.
> 
> Agreed, that parsing substrings of fuller datetime literal (parse_date,
> parse_time, and parse_zone) are not that much useful without separate
> entities date, time, and zone. I planned to extend usage of those api while
> working with SQL datetime support and it's runtime. At the moment we have
> datetime entity, and time without date is not yet making much sense.
> 
> Suggestions, for a moment - disable/hide parse_time() and parse_zone().
> 
> parse_date() is perfectly legal now, it will use the same datastructures
> where there will be empty information for seconds/nanoseconds since
> beginning of a date.

I'd prefer parse() to handle the case when there's no time in the string
if possible, but this is discussable. In fact, I'd prefer not to have
parse() at all - I'd like datetime(str) to handle construction from any
string, with or without time.

> >   - datetime.strftime, asctime, ctime - look too low level, which is no
> >     surprise as they come from the C API. Judging by their names I can't
> >     say that they present a datetime as a string in a certain format.
> >     Maybe, rename strftime() to format() and make it a method of datetime
> >     object (datetime:format)? Without a format, it could print ASCII
> >     time. As for asctime, ctime, I'd drop them, because one can use
> >     strftime to get the same result. Besides, they append \n to the
> >     output, which looks weird:
> > 
> >     tarantool> datetime.asctime(dt)
> >     ---
> >     - 'Thu Jan  1 10:00:00 1970
> >       '
> >     ...
> 
> This is the way asctime/ctime were formatting date for centuries :)
> 
> Agreed though, that their predefined format looks strange, and almost
> useless. And strftime is much more flexible. Will hide/delete their APIs.
> 
> The question is how different output of datetime:format() [without specified
> format] should look like to the way we display by default
> tostring(datetime)?
> 
> And should we introduce format() when we have tostring()?
> 
> [I still prefer it named strftime, because it's becoming clearly apparent
> from the name what format should be used and how]

Guess strftime is fine. Python, uses strftime. Maybe, there's other Lua
APIs that have similar functionality (converting to string with options)
we could copycat?