Cachex function-names break Elixir naming convention

[michielfbr]

Elixir has a naming convention:

Functions that return a boolean are named with a trailing question mark.

But I noticed that several Cachex functions, with a trailing ?, don't return booleans. Instead they return booleans in an ok-tuple. Ie. {:ok, false}
I haven't checked every module, but in the main module I'm talking about Cachex.empty?/2 and Cachex.exists?/3.

In tests with non-existing cache, this results in false positives/negatives with assertions like these:
assert Cachex.exists?(:my_cache, :some_key)
refute Cachex.empty?(:my_cache)

I would suggest either returning plain booleans or removing the question-marks in the function name. Or even implement both flavours. I'd also be happy to submit a PR going for either solution, but first would like to hear others opinion on this.

Looking forward to your reactions!

[whitfin]

Hi @michielfbr!

I originally wrote a huge reply to this but scrapped it to be more respectful of your time 😅. It's still fairly long, but I appreciate your thoughts on any of this! I broke it up into responding to your comments + my own, to try make it easier to follow.

Response

I am well aware of the problem; it was a hot topic on Twitter recently. Unfortunately, Twitter is quite simply the worst medium possible for actually having a conversation, so I'm glad you brought it to GitHub! FWIW it was polled there (not by me, so unbiased!) and the result was that it should not be changed.

Having said that, I am totally willing to find common ground here - but there are concerns to address. What I want is a solution that takes the users into account. It's very easy to point at convention and say "your library sucks, fix this", but context does matter.

I noticed that several Cachex functions, with a trailing ?, don't return boolean

The core reason I chose ? is that they do return booleans. The documented API contract of Cachex is that every function in the main module returns { :ok, return_value }. Special casing only these functions impacts consistency, and would be a case of convention > usability. The fact that nobody cared about this to file an issue until now supports this, IMO.

In tests with non-existing cache, this results in false positives/negatives with assertions like these

That being said, you are correct that it can be misleading in the case of assert or things like if, etc. IMO this is a result of people caring about conventions for the wrong reasons. Conventions exist to show intent, they are not a replacement for things like documentation and types. Reminder that "convention" is defined as "a way in which something is usually done". If the Elixir team wanted it to be always, they should have the compiler enforce it.

I would suggest either returning plain booleans or removing the question-marks in the function name. Or even implement both flavours

Yeah, these are the candidates I looked at so far. I originally wrote a full explanation of this, but it was largely pointless so I can summarize it:

• Changing the type introduces inconsistency across the API
• Removing the ? changes the meaning; we need new names
  • Naming matters far more than convention, I don't think anyone disagrees with this
  • Something like Cachex.empty/2 reads as "empty the cache" not "is the cache empty"
  • A new name needs to fit the existing API design, e.g. no Cachex.does_cache_have_entries/2
• Doing both is probably off the table, unfortunately
  • There are people who care about convention, and those who don't
  • Nobody is advocating for the current API, they just don't care about convention
  • So doing both is redundant, while also introducing inconsistency across the API

Instead, rather than arguing about booleans specifically, I think we should be talking about revisiting the API convention in general. Maybe people didn't want to say "change all of it", but that's more compelling than just special casing booleans.

Proposal

I wrote the first version of the library almost a decade ago; Elixir was very new and not widely used. Nobody cared about convention, because the language was still growing. At the same time the approach I chose was also not based on the current Elixir audience, so maybe it's wrong in 2024! I defined the API because there is a case where a cache does not exist. So something like this:

Cachex.get(:missing_cache, "key")

I did not believe that crashing was the best approach here (I still don't, but I could maybe be convinced). So simply because of this, every call had to support { :error, reason } at a minimum for this case. This is where the ? boolean functions broke, all these years ago :)

So perhaps in 2024 this is simply the time to go through the API and do the following:

• Replace the missing cache error with a crash
• Remove the error cases for all infallible functions (i.e. boolean functions)
• Remove the {:ok, return_value} wrapping for functions where appropriate (some calls may still make sense to have this)

For the last point, I don't really like not having :ok tuples in some cases. Being able to eagerly handle your found value is much better (IMO). Consider these two examples:

# removing the {:ok, _} syntax on fetch
case Cachex.fetch(:cache, "key", &String.length/1) do
  {:error, reason} -> 
    # handle error during fetch
    
  {:commit, value} ->
    # handle newly generated values

  value ->
    # handle values already found
end

# keeping the {:ok, _} syntax on fetch
case Cachex.fetch(:cache, "key", &String.length/1) do
  {:ok, value} -> 
    # handle values already found
    
  {:commit, value} ->
    # handle newly generated values

  {:error, reason} -> 
    # handle error during fetch
end

Even in this case though, :ok is vague and kinda sucks. There's probably a better option once it's free of trying to be consistent with the rest of the calls. Side effect of these changes is being able to scrap the nonsense of all the ! generation. It was ugly even 10 years ago!

The problem with this is that the churn is quite high; it will affect literally every function in the Cachex module. For those who don't care about convention (and there's a lot of those people), this is just noise for no benefit. How to balance that is a difficult question. Do I immediately follow with a v5.x after a v4.x just last week? Most likely not.

I'm curious on your thoughts here, so please do let me know!

[whitfin]

As an aside, the conventions you linked would also have me rename Cachex.fetch/4, but I have no interest in doing so. It would also dictate that we label everything with ! because they all have the ability to crash.

Part of the slippery slope of conventions is that you should probably care about them all, or you might as well care about none.

[michielfbr]

Hi @whitfin !

Thank you for sharing your thoughts so extensively!
I'm glad that I am not the first noticing this, thanks for mentioning.
I totally get the objections against simply changing the return of only a couple functions, ignoring the defined API contract, just to match conventions.

I'll move straight to responding to your proposal, trying to keep everything you mentioned in mind. But not before saying that "your library sucks, fix this" surely isn't the message I want to convey. On the contrary.

The 3 changes you list, sound like a good approach to me.

Replace the missing cache error with a crash

Perhaps. Not entirely sure on non-! functions. Maybe return a self-defined exception-struct?
In case of crashing/raising, there might be use for something like Cachex.running?(:my_cache). For cases with un-supervised cache, where one would like to handle the case of a not-running cache.

Remove the error cases for all infallible functions (i.e. boolean functions)

Yes :-)

Remove the {:ok, return_value} wrapping for functions where appropriate (some calls may still make sense to have this)

Yes, but definitely not everywhere. As you say: "only where appropriate"
In general I would say write-to-cache functions would always keep the tuple. Somewhat like Ecto.Repo.
Cachex.fetch() has the potential for writing, so should keep the tuple.

And, although I feel some resistance on bang functions (!), I do think they could be usable next to certain functions, but not all of them.

I realize that any of these changes have major impact on many existing users. Thus I totally get that, if applied, changes like these should be well overthought. And won't happen overnight in a minor patch.