From: Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Vladimir Davydov <vdavydov@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Subject: Re: [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Date: Wed, 18 Aug 2021 16:24:16 +0300 [thread overview] Message-ID: <027a941d-1abc-2ea1-8337-1b79ac5b34d0@tarantool.org> (raw) In-Reply-To: <20210818082537.eyhaa2awaabsje56@esperanza> 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
next prev parent reply other threads:[~2021-08-18 13:24 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-15 23:59 Timur Safin via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 1/8] build: add Christian Hansen c-dt to the build Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:24 ` Safin Timur via Tarantool-patches 2021-08-18 8:56 ` Serge Petrenko via Tarantool-patches 2021-08-17 15:50 ` Vladimir Davydov via Tarantool-patches 2021-08-18 10:04 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 2/8] lua: built-in module datetime Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:30 ` Safin Timur via Tarantool-patches 2021-08-18 8:56 ` Serge Petrenko via Tarantool-patches 2021-08-17 16:52 ` Vladimir Davydov via Tarantool-patches 2021-08-17 19:16 ` Vladimir Davydov via Tarantool-patches 2021-08-18 13:38 ` Safin Timur via Tarantool-patches 2021-08-18 10:03 ` Safin Timur via Tarantool-patches 2021-08-18 10:06 ` Safin Timur via Tarantool-patches 2021-08-18 11:45 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 3/8] lua, datetime: display datetime Timur Safin via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:32 ` Safin Timur via Tarantool-patches 2021-08-17 17:06 ` Vladimir Davydov via Tarantool-patches 2021-08-18 14:10 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 4/8] box, datetime: messagepack support for datetime Timur Safin via Tarantool-patches 2021-08-16 0:20 ` Safin Timur via Tarantool-patches 2021-08-17 12:15 ` Serge Petrenko via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:42 ` Safin Timur via Tarantool-patches 2021-08-18 9:01 ` Serge Petrenko via Tarantool-patches 2021-08-17 18:36 ` Vladimir Davydov via Tarantool-patches 2021-08-18 14:27 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 5/8] box, datetime: datetime comparison for indices Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:43 ` Safin Timur via Tarantool-patches 2021-08-18 9:03 ` Serge Petrenko via Tarantool-patches 2021-08-17 19:05 ` Vladimir Davydov via Tarantool-patches 2021-08-18 17:18 ` Safin Timur via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 6/8] lua, datetime: time intervals support Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:44 ` Safin Timur via Tarantool-patches 2021-08-17 18:52 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 7/8] datetime: perf test for datetime parser Timur Safin via Tarantool-patches 2021-08-17 19:13 ` Vladimir Davydov via Tarantool-patches 2021-08-15 23:59 ` [Tarantool-patches] [PATCH v5 8/8] datetime: changelog for datetime module Timur Safin via Tarantool-patches 2021-08-17 12:16 ` Serge Petrenko via Tarantool-patches 2021-08-17 23:44 ` Safin Timur via Tarantool-patches 2021-08-18 9:04 ` Serge Petrenko via Tarantool-patches 2021-08-17 12:15 ` [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation Serge Petrenko via Tarantool-patches [not found] ` <20210818082222.mofgheciutpipelz@esperanza> 2021-08-18 8:25 ` Vladimir Davydov via Tarantool-patches 2021-08-18 13:24 ` Safin Timur via Tarantool-patches [this message] 2021-08-18 14:22 ` Vladimir Davydov via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=027a941d-1abc-2ea1-8337-1b79ac5b34d0@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=tsafin@tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --cc=vdavydov@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox