whitfin / cachex

A powerful caching library for Elixir with support for transactions, fallbacks and expirations
https://hexdocs.pm/cachex/
MIT License
1.6k stars 103 forks source link

Dialyzer fixes #324

Closed feld closed 8 months ago

feld commented 9 months ago

Here's a collection of dialyzer fixes. The commit logs include the original error message.

There are a few more to clean up but they are either related to @spec being wrong for defmacro in lib/cachex/spec.ex which should all be set to return Macro.t when they return a quoted value, but that obscures the underlying intention if you wanted to check the code/docs quickly. Not sure what to do with that so it has been left alone.

I opted to fix the @spec definitions to use the fully qualified type name (e.g., Cachex.Spec.spec()) instead of relying on the imported Cachex.Spec for clarity, but in those cases it really could have been shortened to just the spec name.

feld commented 9 months ago

btw I don't care if you want to throw away any of the changes like the && removal and only keep the commits that clean up @specs or whatever; I kept each change isolated for easier review and cherry-picking. Do what you wish with these, it was more of a personal exercise in tracking down some Dialyzer errors

whitfin commented 8 months ago

Hi @feld !

Thanks for this, somehow I thought I'd already written on here but it appears I didn't so I'm sorry for that.

I'm definitely happy to include all of the @spec updates. Little bit weary of the single [] change, but I can bench it and see if it's at all relevant (hopefully it's simply not - it shouldn't be).

I personally don't care about dialyzer complaining about && at all; the repo conforms to mix format and IMO that's good enough. Just to make sure... that doesn't cause issues for your dialyzer runs (outside of running inside this repo), right?

In this case there's only a couple so maybe I just accept it to save people a headache, it's not a big deal. I hope to get this merged tonight either way!

whitfin commented 8 months ago

I'm just going to accept as is, there's no real reason not to. I'll run dialyzer locally and double check everything after the fact. Thanks again @feld !

feld commented 7 months ago

that doesn't cause issues for your dialyzer runs (outside of running inside this repo), right?

correct