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"})
next prev parent 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