Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dd/359/do_with_docker_v3 #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

captainenhancement
Copy link

what is new comparing with prev pull req: code was changed following coding-guidelines

-- See file `COPYRIGHT` for the license
--------------------------------------------------------------------------------

require 'lua-aplicado.module' -- for import()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proper way to say that is require 'lua-nucleo'.

However, this tool implicitly depends on le-run-lua-module, which already makes import available.

Please, either change the whole pk-test codebase to explicitly depend on lua-nucleo, or drop this line.

--------------------------------------------------------------------------------

require 'lua-aplicado.module' -- for import()
require 'lfs' -- for lfs.chdir(), lfs.currentdir()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guidelines dictate that you must cache the module locally. (Please make sure that the guidelines indeed say so explicitly, send a PR to fix if they do not.)

local lfs = require 'lfs'


-----------------------------------------------------------------------

--[[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use ldoc formatting

how does function handle errors:
function currently crashes with calling error() on
any error.
function returns true on success. ]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

]] violates the guidelines

function currently crashes with calling error() on
any error.
function returns true on success. ]]
local function do_with_docker (cfg_dir, handler)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line violates the guidelines (in several ways)

-- so look on functions as on named-blocks-for-using-in-'if'.
local cur_dir

local crash = function(err_msg)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fail is more common name in our code for such functions, please use it

crash('cannot enter cfg dir')
end
if not start_container() then
restore_dir()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not exception-safe. do use a *pcall function to guarantee the directory is restored no matter what

crash('cannot start container')
end
if not restore_dir() then
stop_container()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. *pcall is your friend

-- call handler()
log('do_with_docker(): executing handler()')
if not exec_handler() then
local _ =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too much magic. use ifs

{
do_with_docker = do_with_docker;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants