[tarantool-patches] Re: [PATCH v2 3/3] lua-yaml: treat an empty document/value as null

[Alexander Turenko]

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.