Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate
Date: Sat, 10 Oct 2020 04:19:03 +0300	[thread overview]
Message-ID: <20201010011903.dwljzqfr3yyiidvw@tkn_work_nb> (raw)
In-Reply-To: <a898519b-d4e4-6ccf-a5e1-d24d8b17984a@tarantool.org>

On Fri, Oct 09, 2020 at 10:11:05PM +0200, Vladislav Shpilevoy wrote:
> 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 <type>, then the <type> argument goes first,
> >> and the method name is <type>_<action>.
> >>
> >> 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.

Aye!

> 
> >> 	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_<action>()' (generally, see below).
> 
> Tuple validation methods operate on key_def in the same extent as
> on the tuples.

Yea, I just tried to find a more precise pattern in the internal naming
that may be useful for us here.

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

Looks meagingful for me. Since it anyway breaks the attempt to use
'box_tuple_<action>()' naming for keydefish actions on tuples, I would
also choose box_key_def_extract_key() instead of
box_tuple_extract_key_v2().

The result becomes the same as in the first table above :)

 | 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_key_def_validate_tuple()   |
 | box_tuple_compare()            |
 | box_tuple_compare_with_key()   |
 | box_key_def_extract_key()      |
 | box_key_def_validate_key()     |

All names are prefixed, most with the same prefix (except
box_tuple_compare*()). Okay for me. I'll update my patchset to follow
this agreement.

  reply	other threads:[~2020-10-10  1:18 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-24 17:00 [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate Timur Safin
2020-09-28 22:20   ` Vladislav Shpilevoy
2020-10-02 12:24     ` Timur Safin
2020-10-09  1:11     ` Alexander Turenko
2020-10-09 20:11       ` Vladislav Shpilevoy
2020-10-10  1:19         ` Alexander Turenko [this message]
2020-09-29  5:25   ` Alexander Turenko
2020-10-05  7:35     ` Alexander Turenko
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 2/7] module api: export box_key_def_dup Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:03     ` Alexander Turenko
2020-09-29 23:19       ` Vladislav Shpilevoy
2020-10-01  3:05         ` Alexander Turenko
2020-10-02 12:25           ` Timur Safin
2020-10-02 12:26     ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 3/7] module api: luaT_newthread Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-10-02 12:27     ` Timur Safin
2020-10-02 21:48       ` Vladislav Shpilevoy
2020-09-29  6:25   ` Alexander Turenko
2020-10-02 12:26     ` Timur Safin
2020-10-02 12:53       ` Alexander Turenko
2020-09-29 15:19   ` Igor Munkin
2020-10-02 16:12     ` Timur Safin
2020-10-03 16:57       ` Igor Munkin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 4/7] module api: luaL_register_module & luaL_register_type Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:09     ` Alexander Turenko
2020-09-29 23:20       ` Vladislav Shpilevoy
2020-09-30  6:31         ` Alexander Turenko
2020-09-30  6:33           ` Alexander Turenko
2020-10-02 16:14       ` Timur Safin
2020-09-29  8:03     ` Igor Munkin
2020-09-29 23:21       ` Vladislav Shpilevoy
2020-10-02 16:14       ` Timur Safin
2020-10-03  3:24         ` Alexander Turenko
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 5/7] module api: luaT_temp_luastate & luaT_release_temp_luastate Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:17     ` Alexander Turenko
2020-09-29 23:21       ` Vladislav Shpilevoy
2020-10-01  3:35         ` Alexander Turenko
2020-09-29 15:10     ` Igor Munkin
2020-09-29 21:03       ` Alexander Turenko
2020-09-29 23:23         ` Vladislav Shpilevoy
2020-09-30 10:09           ` Alexander Turenko
2020-10-01 15:06             ` Igor Munkin
2020-10-03  2:16         ` Alexander Turenko
2020-10-02 12:24       ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 6/7] module api: luaL_checkibuf & luaL_checkconstchar Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:53     ` Alexander Turenko
2020-09-29 23:25       ` Vladislav Shpilevoy
2020-10-01  3:00         ` Alexander Turenko
2020-10-02 16:14           ` Timur Safin
2020-10-02 21:48             ` Vladislav Shpilevoy
2020-10-08 13:50           ` Timur Safin
2020-09-24 17:00 ` [Tarantool-patches] [PATCH 2.X 7/7] module api: luaL_cdata_iscallable Timur Safin
2020-09-28 22:21   ` Vladislav Shpilevoy
2020-09-29  5:19     ` Alexander Turenko
2020-10-02 16:14     ` Timur Safin
2020-10-02 12:23 ` [Tarantool-patches] [PATCH 2.X 0/7] RFC: module api: extend for external merger Lua module Timur Safin
2020-10-02 21:49   ` Vladislav Shpilevoy
2020-10-03  2:54   ` Alexander Turenko

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=20201010011903.dwljzqfr3yyiidvw@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate' \
    /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