From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Sun, 9 Dec 2018 12:36:00 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name
 in all fio errors
Message-ID: <20181209093600.qskbqppb3a3n53zq@esperanza>
References: <cover.1543938959.git.roman.habibov@tarantool.org>
 <20181204161239.3ngth5tqnt33ixk2@esperanza>
 <20181207072535.72jfepnrjj44vfnw@tkn_work_nb>
 <565504fd-61a3-092a-c9df-98b93c064a3c@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <565504fd-61a3-092a-c9df-98b93c064a3c@tarantool.org>
To: roman <roman.habibov@tarantool.org>
Cc: tarantool-patches@freelists.org, Alexander Turenko <alexander.turenko@tarantool.org>, Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
List-ID: <tarantool-patches.dev.tarantool.org>

On Sat, Dec 08, 2018 at 04:41:08PM +0300, roman wrote:
> diff --git a/src/lua/fio.lua b/src/lua/fio.lua
> index 55faebdcb..2fee13e2d 100644
> --- a/src/lua/fio.lua
> +++ b/src/lua/fio.lua
> @@ -188,7 +188,7 @@ fio.open =3D function(path, flags, mode)
> =A0=A0=A0=A0 for _, m in pairs(mode) do
> =A0=A0=A0=A0=A0=A0=A0=A0 if type(m) =3D=3D 'string' then
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if fio.c.mode[m] =3D=3D nil then
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 error(sprintf("Unknown mod=
e: %s", m))
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 error(sprintf("fio.open():=
 unknown mode: %s", m))

Here you use a colon (:) ...

> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 end
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 imode =3D bit.bor(imode, fio.c.mode[=
m])
> =A0=A0=A0=A0=A0=A0=A0=A0 else
> @@ -213,7 +213,7 @@ fio.pathjoin =3D function(...)
> =A0=A0=A0=A0 while i <=3D len do
> =A0=A0=A0=A0=A0=A0=A0=A0 local sp =3D select(i, ...)
> =A0=A0=A0=A0=A0=A0=A0=A0 if sp =3D=3D nil then
> -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 error("Undefined path part "..i)
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 error("fio.pathjoin() undefined path p=
art "..i)

... while here you don't. Please be consistent throughout the code.

Also, there's already some inconsistency among existing error messages:

        error("Usage open(path[, flags[, mode]])")
	             ^^^^ fio prefix missing

        error("Usage fio.basename(path[, suffix])")
	           ^^^ semicolon missing

        error("Usage: fio.chdir(path)")

Please fix them all in the same patch. I think they all should look
like:

        error("Usage: fio.function(args)")

Don't forget to update the commit message accordingly (something like
"fio: cleanup error messages") and reset the author (git commit --amend
--reset-author) - for some reason Vladislav authored this commit.

> diff --git a/test/app/fio.result b/test/app/fio.result
> index b7a1f65c6..38d908276 100644
> --- a/test/app/fio.result
> +++ b/test/app/fio.result
> @@ -55,6 +55,19 @@ fio.pathjoin('abc', 'awdeq///qweqwqwe///', "//asda//")
> =A0---
> =A0- abc/awdeq/qweqwqwe/asda
> =A0...
> +--gh-3580 Modify the error message in 'fio.pathjoin'.

This is a confusing comment IMO. Let's just say something like

-- gh-3580: Check that error messages are descriptive enough.

> +fio.pathjoin(nil)
> +---
> +- error: 'builtin/fio.lua:216: fio.pathjoin() undefined path part 1'
> +...
> +fio.pathjoin('abc', nil)
> +---
> +- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 2'
> +...
> +fio.pathjoin('abc', 'cde', nil)
> +---
> +- error: 'builtin/fio.lua:236: fio.pathjoin() undefined path part 3'
> +...

You patched error messages returned by several fio functions, but tested
only one them. Please check all fio functions while we are at it.

If you agree with all my comments, please don't reply to this email.
Instead send v2 with a proper change log in a new mailing thread.

Thanks.