From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 9D5696EC40; Wed, 18 Aug 2021 16:24:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 9D5696EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629293072; bh=GyjWWbrlfy6upERY3kla8tkM29rsHXZxODL+Lo0ES5g=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=txwQU4GHwUovPnWpSWYjkssQbautAJ+8VhGVIs2WeL8ED6jPp6QAhhOZtXDq0S+nN nkE5BjijLUB0QKA0cqy/uoGsAD6kh/xANkETvcvx1U5FQjPzf54rJ2/5pn9Ypz1FFk qkyOtMXPDqI9GHEDp4KCf8ZYVgO+l6py+YM1eLBI= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 641F06EC40 for ; Wed, 18 Aug 2021 16:24:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 641F06EC40 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1mGLYS-0004u3-CP; Wed, 18 Aug 2021 16:24:28 +0300 To: Vladimir Davydov References: <20210818082222.mofgheciutpipelz@esperanza> <20210818082537.eyhaa2awaabsje56@esperanza> Message-ID: <027a941d-1abc-2ea1-8337-1b79ac5b34d0@tarantool.org> Date: Wed, 18 Aug 2021 16:24:16 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210818082537.eyhaa2awaabsje56@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9ECFD080E047A606F56291F8739A291D6182A05F538085040EF4080900C0C63FA6DDD7B26D7559438B17AC83FA34C1F58FF05D9CFB5DB269E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7FCFCB92DA8654BB0EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372BE3E2E75E3847F48638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D846CF4DD85F82DFD6740E04B95B384935117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735209ECD01F8117BC8BEA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FCB403722D27F53E293AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F7900637BECDE000F464C10CD81D268191BDAD3D698AB9A7B718F8C4D1B931868CE1C5781A620F70A64A45A98AA50765F79006372E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E625242539A7722CA490CB5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5CA5F6EAC48A377FF77F5CBEA4B4E335883B901541632C217D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA757E10A58996CBD514410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AC632F0BE69382F30C8D5B6C7A32026E8F01F8326A9F4942B3BACB3E50BB55E8DADE2341490B91331D7E09C32AA3244C6010091AEC2FF2B63ED10FB9BACDAA7551E887DA02A9F7BF8D5DD81C2BAB7D1D X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojuRQ/H5n28tp8bAL1zAAUbQ== X-Mailru-Sender: 6CA451E36783D721CBEA96CEA26D325D9B03B3F0046D83FC86E381DCD695921BB7CBEF92542CD7C82F97C478340294DCC77752E0C033A69E0F0C7111264B8915FF1320A92A5534336C18EFA0BB12DBB0 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 0/8] Initial datetime implementation X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Safin Timur via Tarantool-patches Reply-To: Safin Timur Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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