From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Timur Safin <tsafin@tarantool.org> Cc: alexander.turenko@tarantool.org, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Date: Sun, 20 Jun 2021 21:57:59 +0300 [thread overview] Message-ID: <20210620185759.GC10212@tarantool.org> (raw) In-Reply-To: <d3639649921666f45b4110630ad4515b253b3675.1623396615.git.tsafin@tarantool.org> Timur, Thanks for the patch! Though the change is quite trivial, the patch is not OK right from the top. Please adjust the commit subject according to our contribution guidelines[1]. Regarding the changes itself: why did you send it within absolutely unrelated changeset? This is not required for the following patches. This is not related to SQL at all. This patch should be on the another branch, and the next patches should be rebased on that branch (if you need it so much). You can argue that this is a trivial oneliner and I am making lot of ado about nothing, *but* this is your point of view. And now let me share mine one with you: * This patch should be backported to all longterm branches, since I consider this as a bug. Since this patch is on the bottom of the series, I need to make more actions for this. For what? I see no reason for it. * You've chosen Sasha as a *first* reviewer for the first patch of the series. However, I'm sure every member of our "L" team can carefully review such a simple fix. Hence, you decided to stall the whole series until Sasha approves this tiny patch? * On the other hand if this patch has enough LGTMs, it can be applied "out of order". And when it is applied, you will need to rebase your working branch with SQL-related changes on master. As a result this *optimization* has no sense at all and only makes maintainer's life much worse. Finally, if there was not a nit regarding this patch, I would suggest you a deal: leave the patch as is and consider everything written above for the further patches. However, there are several nits to be resolved, so please move this patch out from this series in a separate one. On 11.06.21, Timur Safin via Tarantool-patches wrote: > It always was a problem that reported source line was not > pointing to the actual callee line number, but rather to Don't get why you are talking about callee here: regardless the line to be reported (the current one or the one where the function is defined), it is a *caller* for the <traceback> function. So, the actual problem you're writing is that the line with function signature is used instead of the line where the function execution is stopped at the moment of the <traceback> call. As for me, these are the different reasons. > the start of file, i.e. we have seen: > ``` > [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx > [001] not ok 22 - 4.3 # > [001] Traceback: > [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123> > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0> > [001] > [001] not ok 23 - 4.4 # > [001] Traceback: > [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:123> > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:0> > ``` > (see the :0 part) Minor: Strictly saying :123 part is also broken. The only difference between them is that :0 line is used for so called "main" function. > Instead of correct line numbers: > ``` > [001] sql-tap/tkt-9a8b09f8e6.test.lua memtx > [001] not ok 22 - 4.3 # > [001] Traceback: > [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142> > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:242> > [001] > [001] not ok 23 - 4.4 # > [001] Traceback: > [001] [Lua ] function 'do_catchsql_test' at </home/tsafin/tarantool/test/var/001_sql-tap/sqltester.lua:142> > [001] [main] at </home/tsafin/tarantool/test/sql-tap/tkt-9a8b09f8e6.test.lua:252> > ``` > > The problem was due to `.linedefined` used, instead of source line in `.currentline`. Typo: The line exceeds 72 chars. > > Closes #6134 > --- > src/lua/tap.lua | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lua/tap.lua b/src/lua/tap.lua > index 346724d84..77fd8d096 100644 > --- a/src/lua/tap.lua > +++ b/src/lua/tap.lua > @@ -23,7 +23,7 @@ local function traceback(level) > local frame = { > source = info.source; > src = info.short_src; > - line = info.linedefined or 0; > + line = info.currentline or info.linedefined or 0; Why did you leave such a complex code here? I believe you can use just info.currentline here. > what = info.what; > name = info.name; > namewhat = info.namewhat; > -- > 2.29.2 > [1]: https://www.tarantool.io/en/doc/latest/dev_guide/developer_guidelines/#how-to-write-a-commit-message -- Best regards, IM
next prev parent reply other threads:[~2021-06-20 18:58 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-11 7:48 [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Timur Safin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines Timur Safin via Tarantool-patches 2021-06-20 18:57 ` Igor Munkin via Tarantool-patches [this message] 2021-06-23 21:01 ` Alexander Turenko via Tarantool-patches 2021-06-27 23:16 ` Timur Safin via Tarantool-patches 2021-06-29 16:21 ` Igor Munkin via Tarantool-patches 2021-06-30 6:49 ` Timur Safin via Tarantool-patches 2021-07-21 7:24 ` Igor Munkin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 2/3] sql: updated explicit conversion table Timur Safin via Tarantool-patches 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches 2021-06-25 21:26 ` Timur Safin via Tarantool-patches 2021-06-25 21:26 ` [Tarantool-patches] Отзыв: " Timur Safin via Tarantool-patches 2021-06-27 23:46 ` [Tarantool-patches] " Timur Safin via Tarantool-patches 2021-06-11 7:48 ` [Tarantool-patches] [PATCH v2 3/3] sql: updated implicit " Timur Safin via Tarantool-patches 2021-06-20 18:52 ` Mergen Imeev via Tarantool-patches 2021-06-28 0:06 ` Timur Safin via Tarantool-patches 2021-06-20 18:52 ` [Tarantool-patches] [PATCH v2 0/3] sql: modify explicit and implicit conversion tables Mergen Imeev via Tarantool-patches 2021-06-27 23:29 ` Timur Safin via Tarantool-patches
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=20210620185759.GC10212@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=alexander.turenko@tarantool.org \ --cc=imun@tarantool.org \ --cc=tsafin@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/3] test: corrected reported error lines' \ /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