From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D5E7724FB3 for ; Mon, 4 Feb 2019 22:30:05 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4OYe6MGUAFoZ for ; Mon, 4 Feb 2019 22:30:05 -0500 (EST) Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8E9F224F35 for ; Mon, 4 Feb 2019 22:30:05 -0500 (EST) Date: Tue, 5 Feb 2019 06:30:01 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v2 3/3] lua-yaml: treat an empty document/value as null Message-ID: <20190205033000.omywtlgxzpbcplgx@tkn_work_nb> References: <0a841ae0-f03f-5406-5a2e-d3fc81843784@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <0a841ae0-f03f-5406-5a2e-d3fc81843784@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org Hi! Thanks. Your scepsis pushed me to rethink the change and I decided to discard this patch. See below for more info. WBR, Alexander Turenko. On Fri, Jan 25, 2019 at 12:26:49AM +0300, Vladislav Shpilevoy wrote: > Thanks for the patch! > > On 22/01/2019 05:12, Alexander Turenko wrote: > > It improves our compatibility with the YAML standard. > > > > yaml.encode('') result changed from > > > > --- > > ... > > > > to > > > > --- '' > > ... > > > > yaml.decode('') returns zero values count before, now it returns > > box.NULL. yaml.decode('- ') returns {''} before, now {box.NULL}. > > > > This commit touches many tests and test result files, which use console, > > without behaviour changes. It also adds two test cases to > > app-tap/console.test.lua. > > Not sure if it makes sense listing factual changes here, because > literally the same I see below and in "git diff --stat". The idea of the message was to mark which test changes are just due to the change and which one are specifically designed to test the new feature. I tried to clarify that and would rewrite as follows (but dropped the patch, see below). ``` Several test cases were added to the app-tap/console.test.lua test, other test and test result changes are just induced by the encode/decode behaviour change. ``` > > > --- > > test/app-tap/console.test.lua | 16 +- > > test/app/fio.result | 2 +- > > test/app/socket.result | 26 +- > > test/box/access.result | 8 +- > > test/box/role.result | 8 +- > > test/box/sequence.result | 2 +- > > test/vinyl/errinj_tx.result | 10 +- > > test/vinyl/hermitage.result | 122 ++-- > > test/vinyl/mvcc.result | 1066 +++++++++++++++---------------- > > test/vinyl/mvcc.test.lua | 4 +- > > test/vinyl/tx_conflict.result | 2 +- > > test/vinyl/tx_conflict.test.lua | 2 +- > > test/vinyl/tx_gap_lock.result | 189 +++--- > > test/vinyl/tx_gap_lock.test.lua | 3 +- > > test/vinyl/tx_serial.result | 2 +- > > test/vinyl/tx_serial.test.lua | 2 +- > > third_party/lua-yaml/lyaml.cc | 22 +- > > 17 files changed, 747 insertions(+), 739 deletions(-) > > > > diff --git a/test/vinyl/mvcc.result b/test/vinyl/mvcc.result > > index 1941744b9..db7fc8e83 100644 > > --- a/test/vinyl/mvcc.result > > +++ b/test/vinyl/mvcc.result > > @@ -41,29 +41,29 @@ t = box.space.test > > -- > > c1:begin() > > --- > > -- > > +- null > > How about to start returning nothing instead of returning null so > as to do not change the tests (or at least make the diff smaller)? > > I see, that most of the diff is from txn_proxy, which always does > return 'something'. You can change it so as to do not just return, but > check if a result is not null, and only then return. Otherwise do nothing > or empty return. > > I am sure it will work since space:get(), for example, still can > return nothing (!= null), and this is why other tests didn't fail. Changing tests is not the main question here. The question is what is the right thing to do with encoding of an empty document. It is null according to the standard. But our approach allows to encode null and empty values set differently. I factored out this patch from the Alex's Kh. original commit to discuss it separately. After a yesterday discussion with Vladimir I understood that current approach has its benefit (see above). So there are no reason to change it at least until it will be requested by someone. I dropped the patch from the patchset.