Skip to content

Conversation

@ypnos
Copy link
Contributor

@ypnos ypnos commented Feb 22, 2021

Current messages for minimum and maximum were misleading for a wide audience. Messages are now changed to "at least" for minimum and "at most" for maximum.
Code simplified.

Current messages for `minimum` and `maximum` were misleading for a wide audience. Messages are now changed to "at least" for `minimum` and "at most" for `maximum`.
Code simplified.
ctx:stmt(sformat(' if %s %s %s then', ctx:param(1), op, schema.minimum))
ctx:stmt(sformat(' return false, %s("expected %%s to be %s than %s", %s)',
ctx:libfunc('string.format'), msg, schema.minimum, ctx:param(1)))
local addRangeCheck = function (op, reference, msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

This way of writing has a bad performance. You can use this way:

local function(ctx, op, reference, msg)
   ... ...
end

generate_validator = function(ctx, schema)
    ...
    if schema.minimum then
      addRangeCheck(ctx, '<', schema.minimum, 'at least')
    end
    ...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for delay. I incorporated your improvement.

Copy link
Contributor

@membphis membphis left a comment

Choose a reason for hiding this comment

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

please change to new way

@membphis
Copy link
Contributor

ping @ypnos

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2021

CLA assistant check
All committers have signed the CLA.

@ypnos ypnos changed the title Clarify numerical validation errors Feat: Clarify numerical validation errors Apr 19, 2021
@ypnos ypnos changed the title Feat: Clarify numerical validation errors feat: Clarify numerical validation errors Apr 19, 2021
@membphis
Copy link
Contributor

@spacewander do you have time to look at this PR? many thx

@spacewander spacewander merged commit 62c1408 into api7:master Apr 20, 2021
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.

4 participants