Re: [tarantool-patches] Re: [PATCH 0/2 v2] fio: show function name in all fio errors

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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 = function(path, flags, mode)
>      for _, m in pairs(mode) do
>          if type(m) == 'string' then
>              if fio.c.mode[m] == nil then
> -                error(sprintf("Unknown mode: %s", m))
> +                error(sprintf("fio.open(): unknown mode: %s", m))

Here you use a colon (:) ...

>              end
>              imode = bit.bor(imode, fio.c.mode[m])
>          else
> @@ -213,7 +213,7 @@ fio.pathjoin = function(...)
>      while i <= len do
>          local sp = select(i, ...)
>          if sp == nil then
> -            error("Undefined path part "..i)
> +            error("fio.pathjoin() undefined path part "..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//")
>  ---
>  - abc/awdeq/qweqwqwe/asda
>  ...
> +--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.