[Tarantool-patches] [PATCH 2.X 1/7] module api: export box_tuple_validate

[Vladislav Shpilevoy]

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.

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

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