From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 98C25469719 for ; Fri, 9 Oct 2020 04:22:36 +0300 (MSK) Date: Fri, 9 Oct 2020 04:22:53 +0300 From: Alexander Turenko Message-ID: <20201009012253.z43gxxrmtxfedksk@tkn_work_nb> References: <501646b13840c7f8166e47c7ce75385b76dc00b0.1600817803.git.alexander.turenko@tarantool.org> <9ed17785-7743-827f-2fb3-d38627311699@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9ed17785-7743-827f-2fb3-d38627311699@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 14/14] WIP: module api: add box_key_def_validate_key() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org > > +int > > +box_key_def_validate_key(const box_key_def_t *key_def, const char *key, > > + bool allow_nullable) > > I don't like exposing such internal parameters as 'allow_nullable'. It > took me some time to find why is it even needed. Maybe better export 2 > separate functions? > > box_key_def_validate_exact_key > box_key_def_validate_range_key > > Or > > box_key_def_validate_unique_key > box_key_def_validate_key > > Or > > box_key_def_validate_unique_key > box_key_def_validate_non_unique_key > > Or > > box_key_def_validate_primary_key > box_key_def_validate_secondary_key > > Also did you think about making this function not setting any error? > What if a module code does not want to spoil diag, and just wants > to check the key? So as to set an own error later. Maybe would be > more flexible to expose a simple 'is_valid' instead of 'validate'? To be honest, I don't like 'allow_nullable' name too. I had guess that it would be more intuitive than 'is_exact', but it seems, it is not. In fact, the flag is like a hack to validate a key against a key_def as if all parts would be defined as non-nullable. Nothing prevents a user to create the same key_def, but non-nullable (_dump_parts(), change nullability of parts, call _new_v2()). So I will just remove the flag. If we'll really need it, we surely will able to add a function that will be named using words 'exact' and 'key'. Aside of this, please, look at the discussion around names of tuple and key_def related functions in [1]. It relates to this function as well. [1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/019989.html > > Or do we do that to get a more precise error message and this is > more important than not setting a diag when not needed? I would say 'yes'. The diagnostic area is a kind of errno and I don't see anything bad in filling it even when not any caller will read the error.