Tarantool development patches archive
 help / color / mirror / Atom feed
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
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

  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 \
    /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

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git