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

[Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.

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.

> 
>   - 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.

> 
>   - 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.

> 
>   - datetime.days(2) + datetime.hours(12) + datetime.minutes(30)
> 
>     looks cool, but I'm not sure it's efficient to create so many objects
>     and then sum them to construct an interval. It's surely acceptable
>     for compiled languages without garbage collection (like C++), but for
>     Lua I think it'd be more efficient to provide an API like this:
> 
>     datetime.interval{days = 2, hours = 12, minutes = 30}

I do have such API also, e.g. date:add{ days = 2, hours = 12, minutes = 
30} they do modify original object though.

	tarantool> date.now():add{days = 2, hours = 2}
	---
	- 2021-08-20T18:17:13.897261+03:00
	...

> 
>     (I'm not a Lua expert so not sure about this)
> 
>   - 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]

Thanks,
Timur