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

Fix evaluate_as_string spec #238

Merged
merged 3 commits into from
Dec 12, 2024
Merged

Conversation

maryamaljanabi
Copy link
Contributor

@maryamaljanabi maryamaljanabi commented Dec 11, 2024

Adding Number.t() to the expression types for evaluate_as_string spec. The reason for this is a failure in the main repo when the spec for evaluate_as_string was added. Also the parser allows string and number types @spec parse!(String.t() | Number.t()) :: Keyword.t()

Adding a release 2.41.4

@maryamaljanabi maryamaljanabi added the bug Something isn't working label Dec 11, 2024
@maryamaljanabi maryamaljanabi self-assigned this Dec 11, 2024
smn
smn previously requested changes Dec 11, 2024
Copy link
Contributor

@smn smn left a 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 ?

@@ -120,7 +120,7 @@ defmodule Expression do
end

@spec evaluate_as_string!(
String.t() | nil,
any(),
Copy link
Contributor

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.

Copy link
Contributor Author

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:

@spec parse!(String.t() | Number.t()) :: Keyword.t()

The parse_expression is the one that only accepts a string:
@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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@joaothallis joaothallis self-requested a review December 11, 2024 11:59
@maryamaljanabi
Copy link
Contributor Author

maryamaljanabi commented Dec 11, 2024

Hi @santiagocardo would love some feedback here. I noticed you introduced the ability for parsers to parse numbers in this line

@spec parse!(String.t() | Number.t()) :: Keyword.t()

Does this mean the evaluate_as_string method should also be able to parse numbers? We are discussing that in a slack thread and just wanted to confirm that it's okay to change the type of the input to this method from String.t() | nil to String.t() | Number.t() | nil. I added String.t() | nil a few days ago thinking numbers shouldn't be there but noticed it's there in the parser and also dialyzer is complaining in our codebase in some areas I highlighted in the slack thread when I added that.

@maryamaljanabi maryamaljanabi requested a review from smn December 11, 2024 15:27
@santiagocardo
Copy link
Contributor

Hi @santiagocardo would love some feedback here. I noticed you introduced the ability for parsers to parse numbers in this line

@spec parse!(String.t() | Number.t()) :: Keyword.t()

Does this mean the evaluate_as_string method should also be able to parse numbers? We are discussing that in a slack thread and just wanted to confirm that it's okay to change the type of the input to this method from String.t() | nil to String.t() | Number.t() | nil. I added String.t() | nil a few days ago thinking numbers shouldn't be there but noticed it's there in the parser and also dialyzer is complaining in our codebase in some areas I highlighted in the slack thread when I added that.

Yes, @maryamaljanabi Expression.evaluate_as_string!/1 should also accept numbers because Expression.parse!/1 already accepts numbers:

@spec parse!(String.t() | Number.t()) :: Keyword.t()

So yes, changing that function's type to String.t() | Number.t() | nil is okay. Thanks for fixing it!

@maryamaljanabi
Copy link
Contributor Author

@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!

@smn smn dismissed their stale review December 12, 2024 15:28

Unblocking

@smn
Copy link
Contributor

smn commented Dec 12, 2024

@maryamaljanabi thank you for addressing this and thanks @santiagocardo for weighing in on the number change!

@maryamaljanabi maryamaljanabi merged commit d2b89d7 into develop Dec 12, 2024
3 checks passed
@joaothallis joaothallis deleted the fix/evaluate_as_string-spec branch December 12, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants