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

ensure.lua additions #54

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

ensure.lua additions #54

wants to merge 7 commits into from

Conversation

isage
Copy link

@isage isage commented Mar 5, 2019

  • LDoc
  • Added ensure_fails
  • Added error object support to ensure_fails_with_substring

Depends on #53

@isage isage changed the title ensule.lua additions ensure.lua additions Mar 6, 2019
test/cases/0270-ensure.lua Show resolved Hide resolved
test/cases/0270-ensure.lua Show resolved Hide resolved
local ensure_fails = function(msg, fn)
local res, err = pcall(fn)

if res ~= false then
Copy link
Member

Choose a reason for hiding this comment

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

слишком строгая типизация
if not res

Copy link
Author

Choose a reason for hiding this comment

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

Заметь, там "не равно". Т.е. правильно if res
Во втором месте (ensure_fails_with_substring) тоже поправить?

Copy link
Member

Choose a reason for hiding this comment

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

это хороший вопрос. не знаю. как понять, надо ли?

Copy link
Author

Choose a reason for hiding this comment

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

Ну, логично, что надо, для единообразия)

Copy link
Member

Choose a reason for hiding this comment

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

ОК, допустим

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@isage isage force-pushed the isage/ensure branch 2 times, most recently from 3971714 to 2b3cd6b Compare March 26, 2019 19:52
@@ -14,3 +14,4 @@ Rafis Ganeyev <rafisganeyev@gmail.com>
Mark Gurevich <markgurevichster@gmail.com>
Vladimir Stebunov <vstebunov@gmail.com>
Sergey Ledyankin <sirgeika@gmail.com>
Ivan Epifanov <isage.dna@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

принято, вынести отдельно если этот PR не вольём

if is_error_object(err) then
err = get_error_id(err)
end

if type(err) ~= "string" then
error("ensure_fails_with_substring failed: " .. msg .. ": call failed with non-string error")
Copy link
Member

Choose a reason for hiding this comment

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

принято, дать отдельно если этот PR не вольём

@@ -366,7 +366,7 @@ end
local ensure_fails_with_substring = function(msg, fn, substring)
local res, err = pcall(fn)

if res ~= false then
Copy link
Member

Choose a reason for hiding this comment

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

неправильный и пугающий commit message. переименовать во что-то типа

ensure_fails_with_substring: allow nil as error value

думай о том, как собирать из таких commit message release notes. представь, что у тебя есть только git shortlog и больше никуда нельзя смотреть.

Copy link
Author

Choose a reason for hiding this comment

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

done

@isage isage force-pushed the isage/ensure branch 2 times, most recently from 311c8b2 to 4e1fc39 Compare March 26, 2019 20:06
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.

None yet

2 participants