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

Bug on boolean values #111

Closed
escobera opened this issue Apr 7, 2021 · 3 comments
Closed

Bug on boolean values #111

escobera opened this issue Apr 7, 2021 · 3 comments
Labels

Comments

@escobera
Copy link
Contributor

escobera commented Apr 7, 2021

Hello there,

I have a function similar to this one:

@decorate cacheable(cache: MyCache, key: id)
def good?(id) do
  HttpClient.get() 
  |> handle_response()
end

When this function returns false, even though the value gets cached, the next time it is called it won't use the cached value and issue another http request.

I tracked down the problem to this code:

if value = cache.get(key, opts) do
value
else
Caching.eval_match(unquote(block), match, cache, key, opts)

Problem here is if cache.get(key, opts) evals to false (this is my case) this line value = cache.get(key, opts) evals to false, you can test in iex with:

iex(1)> x = false
false

Since this is not a pure function and the value gets cached I wasn't able to make a failing test case for it, but it seems that to fix this you can change those lines to:

case cache.get(key, opts) do
  nil -> Caching.eval_match(unquote(block), match, cache, key, opts)
  value -> value
end

I ran the tests here after the change and they pass.

@cabol
Copy link
Owner

cabol commented Apr 8, 2021

Thanks for report the issue, question, would you like to send a PR with the fix? Otherwise, NP, I can take a look at it and apply the fix. Let me know, stay tuned! Thanks again!

@escobera
Copy link
Contributor Author

escobera commented Apr 8, 2021

I'm trying to come up with a test case for it. I'll send the PR as soon as possible.

@cabol
Copy link
Owner

cabol commented Apr 8, 2021

The PR LGTM! It is merged, I will publish a new release this weekend! Thank you for fixing the issue 👍 !!

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

No branches or pull requests

2 participants