Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Nikita Tatunov <hollow653@gmail.com>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH] sql: LIKE & GLOB pattern comparison issue
Date: Fri, 27 Jul 2018 23:22:19 +0300	[thread overview]
Message-ID: <20180727202219.ikwbax7tysfnmgr4@tkn_work_nb> (raw)
In-Reply-To: <CAEi+_arMh=rLm7jNAX-ERHjf6GQ5D7RqQK=+wSXCwJjZouoyeA@mail.gmail.com>

Minor comments are below. Looks good for me in general.

Please, review the patch with Lesha after the fixes.

WBR, Alexander Turenko.

> > > With the patch applied an error will be returned in case there's
> > > invalid UTF-8 symbol in pattern & pattern will not be matched
> > > with the string that contains it. Some minor corrections to function
> > > were made as well.
> > >
> >
> > Nitpicking: 'error will be returned' is third situatuon, other then
> > 'matched' and 'not matched'.
> >
> >
> I guess commit message is about changes. Do I really need to
> mention valid cases that didn't change?
> 

Ok, now I got that 'it' is 'invalid symbol'. Misunderstood it before as
description of the one case when a pattern has invalid UTF-8 symbols.
The sentence is correct. Maybe it can be clarified like so: '& correct
UTF-8 pattern will not be matched with a string with invalid UTF-8
symbols'. Or in another way you like.

'Some minor corrections to function were made as well' -- describe
certain behaviour or code changes (like 'fixed infinite loop during
pattern matching and incorrect matching as well') or remove this
abstract sentence.

> > The function itself looks good except lines longer then 80 columns.
> >
> 
> As i said before I don't think breaking these lines will increase
> readability.
> 

There are two little length exceed place in the code, it needs some
refactoring to decrease code blocks nesting level. I think we can lease
it as is for now (if other reviewer don't ask for that).

But comments within the function are worth to fix to fit the limit.

> +test:do_catchsql_set_test(like_test_cases, prefix)
> +
> +-- Invalid testcases.
> +
> +for i, tested_string in ipairs(invalid_testcases) do
> +
> +-- We should raise an error if pattern contains invalid characters.

Again, indent comments to the same level as code in the block.

If a comment is related to several code blocks (with empty lines btw),
then separate it with the empty line from the code (to don't
misinterpret it as related to the first block).

> + local test_name = prefix .. "2." .. tostring(i)
> + local test_itself = "SELECT 'abc' LIKE 'ab" .. tested_string .. "';"
> + test:do_catchsql_test(
> + test_name,
> + test_itself, {
> + -- <test_name>
> + 1, "LIKE or GLOB pattern can only contain UTF-8 characters"
> + -- <test_name>
> + })
> +

Are there reason to format it like so? Maybe just:

test:do_catchsql_test(test_name, test_itself,
    {1, "LIKE or GLOB pattern can only contain UTF-8 characters"})

  reply	other threads:[~2018-07-27 20:22 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-28 12:47 [tarantool-patches] " N.Tatunov
2018-06-28 12:54 ` [tarantool-patches] " Hollow111
2018-07-18  2:43 ` Alexander Turenko
2018-07-18  5:51   ` Alex Khatskevich
2018-07-18 15:24   ` Nikita Tatunov
2018-07-18 15:53     ` Alex Khatskevich
2018-07-18 15:57       ` Nikita Tatunov
2018-07-18 17:10         ` Alexander Turenko
2018-07-19 11:14           ` Nikita Tatunov
2018-07-19 11:56         ` Alex Khatskevich
2018-07-27 11:28           ` Nikita Tatunov
2018-07-27 13:06             ` Alexander Turenko
2018-07-27 19:11               ` Nikita Tatunov
2018-07-27 20:22                 ` Alexander Turenko [this message]
2018-07-31 13:27                   ` Nikita Tatunov
2018-07-31 13:47                     ` Alexander Turenko
2018-08-01 10:35                       ` Nikita Tatunov
2018-08-01 10:51                         ` Nikita Tatunov
2018-08-01 13:56                           ` Alex Khatskevich
2018-08-01 18:10                             ` Nikita Tatunov
2018-08-01 18:14                               ` Nikita Tatunov
2018-08-08 12:38                               ` Alex Khatskevich

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=20180727202219.ikwbax7tysfnmgr4@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=hollow653@gmail.com \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] sql: LIKE & GLOB pattern comparison issue' \
    /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