-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix evaluate_as_string
spec
#238
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem here (wherever this is surfacing) is likely more wherever this function is being called and that allowing non-string, non-nil values. The parse!
function that the expression is supplied to afaik only accepts a string. Fixing the typespec here likely fixes the symptom but not the cause. Where is this problem surfacing and can we start investigating there as well ?
lib/expression.ex
Outdated
@@ -120,7 +120,7 @@ defmodule Expression do | |||
end | |||
|
|||
@spec evaluate_as_string!( | |||
String.t() | nil, | |||
any(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may solve the typespec issue but whatever we're passing in must be string or a nil, otherwise the parse!
on line 133 will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parse
that's used in evaluate_as_string
accepts a string or a number:
Line 86 in 6e35e4b
@spec parse!(String.t() | Number.t()) :: Keyword.t() |
The
parse_expression
is the one that only accepts a string:Line 51 in 6e35e4b
@spec parse_expression(String.t()) :: {:ok, Keyword.t()} | {:error, String.t()} |
The update triggered a dialyzer error in the main repo somewhere that was passing a value for the expression and another method that's consuming that value and expecting it to be binary() | Number.t()
. I can add a spec identical to the parse
one with nil like String.t() | Number.t() | nil
?
Since this is a public repo, not sure if I should paste links to where it's failing. I'll add it on Slack and tag you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced @any
with String.t() | Number.t() | nil
to match the parser and also allow nil which we recently supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smn I have changed Expression.parse!/1
to accept numbers in #129. So Expression.evaluate_as_string!/1
should also accept numbers. In that case, the spec should be String.t() | Number.t() | nil
instead of any()
as @maryamaljanabi did in 0f05eaa.
Hi @santiagocardo would love some feedback here. I noticed you introduced the ability for parsers to parse numbers in this line Line 86 in 6e35e4b
Does this mean the |
Yes, @maryamaljanabi Line 86 in 6e35e4b
So yes, changing that function's type to |
@smn I can't merge due to your open review request and it feels disrespectful to dismiss it. Santiago approved and you can see the discussion above. If you see this, please remove the merge block so I can merge this and go on with resolving the bug fix in the main repo as this is needed there. Thanks! |
@maryamaljanabi thank you for addressing this and thanks @santiagocardo for weighing in on the number change! |
Adding
Number.t()
to the expression types forevaluate_as_string
spec. The reason for this is a failure in the main repo when the spec forevaluate_as_string
was added. Also theparser
allows string and number types@spec parse!(String.t() | Number.t()) :: Keyword.t()
Adding a release
2.41.4