From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 E8F36469719 for ; Fri, 9 Oct 2020 23:11:13 +0300 (MSK) References: <8f0dfb00-0dbe-6717-1c36-90957a072751@tarantool.org> <20201009011102.idolx3rnrk67mt43@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: Date: Fri, 9 Oct 2020 22:11:05 +0200 MIME-Version: 1.0 In-Reply-To: <20201009011102.idolx3rnrk67mt43@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org On 09.10.2020 03:11, Alexander Turenko wrote: >>> +int >>> +box_tuple_validate(box_tuple_format_t *format, box_tuple_t *tuple); >> >> 2. OCD mode on. I would propose either make tuple the first >> argument, or rename it to box_tuple_format_validate_tuple(). >> So as to be consistent with our agreement, that if something >> is a method of , then the argument goes first, >> and the method name is _. >> >> I see we currently have in the public API the functions: >> >> box_tuple_validate - your new function, a bit >> inconsistent. >> >> box_tuple_validate_key_parts - this should have been >> box_key_def_validate_tuple from the beginning, >> but we can't do anything about it now. > > We can. It is part of my patchset. Then lets do it. >> box_key_def_validate_key - correct. Key_def goes first, >> and the name is consistent. >> >> So if you will make box_tuple_validate consistent, we will have >> more correct signatures (2/3) than incorrect, for validation >> methods at least. > > So, if we'll apply all your suggestions, the key_def module API will > contain the following functions: > > | Function | Consumer | Name variants (for history) | > | ---------------------------- | --------------- | ------------------------------- | > | box_key_def_new() | already present | | > | box_key_part_def_create() | key_def module | | > | box_key_def_new_v2() | key_def module | box_key_def_new_ex() | > | box_key_def_dump_parts() | key_def module | | > | box_key_def_merge() | key_def module | | > | box_key_def_dup() | merger module | | > | box_key_def_delete() | already present | | > | box_key_def_validate_tuple() | key_def module | box_tuple_validate_key_parts() | > | box_tuple_compare() | already present | | > | box_tuple_compare_with_key() | already present | | > | box_key_def_extract_key() | key_def module | box_tuple_extract_key_{ex,v2}() | > | box_key_def_validate_key() | key_def module | |> > > All functions around key_defs and tuples are prefixed with 'box_key_def_', > except box_tuple_compare*(), which are already present. > > If we'll follow current internal naming: > > | Function | Name variants (may fit better) | > | ------------------------------ | ------------------------------ | > | box_key_def_new() | | > | box_key_part_def_create() | | > | box_key_def_new_v2() | | > | box_key_def_dump_parts() | | > | box_key_def_merge() | | > | box_key_def_dup() | | > | box_key_def_delete() | | > | box_tuple_validate_key_parts() | box_tuple_validate_key() | > | box_tuple_compare() | | > | box_tuple_compare_with_key() | | > | box_tuple_extract_key_v2() | | > | box_key_def_validate_key() | box_validate_key() | > > Here functions that operate on key_def itself are prefixed with > 'box_key_def_', but functions that operate on tuples using a key > definition are named 'box_tuple_()' (generally, see below). Tuple validation methods operate on key_def in the same extent as on the tuples. > The exception is box_key_def_validate_key(), but we can rename it to > box_validate_key(). And also drop '_parts' from > box_tuple_validate_key_parts() (because it meaningless): > > > | Function | > | ------------------------------ | > | box_key_def_new() | > | box_key_part_def_create() | > | box_key_def_new_v2() | > | box_key_def_dump_parts() | > | box_key_def_merge() | > | box_key_def_dup() | > | box_key_def_delete() | > | box_tuple_validate_key() | > | box_tuple_compare() | > | box_tuple_compare_with_key() | > | box_tuple_extract_key_v2() | > | box_validate_key() | > > Isn't that nice? It is fine. As long as all methods belong to a type and have its name as a prefix. I don't mind if tuple validation and key extraction methods will belong to box_tuple except box_key_def. What looks inconsistent is box_validate_key(). It seems it does not belong to anything. If we rename it to box_key_def_validate_key(), we need to rename box_tuple_validate_key() to box_key_def_validate_tuple() to be consistent in who validates whom. If we rename it to box_key_validate(), then it is inconsistent about not having a 'key' type. And will become wrong if we will ever introduce a key type.