cachex icon indicating copy to clipboard operation
cachex copied to clipboard

Clarify docs and examples for fetch/4

Open SergioInToronto opened this issue 9 months ago • 3 comments

fetch/4

  • Explain behaviour where only 1 instance of lazy evaluation can run simultaneously
  • Shorten explanation where possible
  • Progressive complexity - explain the simplest usage first, adding complexity as the reader keeps reading.

SergioInToronto avatar Mar 25 '25 14:03 SergioInToronto

Hi @SergioInToronto and @HansGlimmerfors! Thanks for taking the time here.

I might be missing something, but I'm not really sure I understand how these changes are an improvement.

Explain behaviour where only 1 instance of lazy evaluation can run simultaneously

I don't think this needs to be covered in a function signature, it already has a dedicated area in documentation. Duplicating the explanation (but with significantly less detail) is an unnecessary extra maintenance burden.

Shorten explanation where possible

Cutting information only to make text smaller doesn't feel necessary to me. For example consider this:

As of Cachex v3.6, you can also provide a third element in a :commit Tuple, to allow passthrough of options from within your fallback. The options supported in this list match the options you can provide to a call of Cachex.put/4. An example is the :expire option to set an expiration from directly inside your fallback.

Which was replaced with:

You may provide a third element in a :commit Tuple, to specify options the same as you would in put/4.

  1. The original documentation provides an example option, the most popular option, so that people likely don't even have to check what is supported.
  2. The original documentation states compatibility; it is clear that Cachex v3.5 does not support this.
  3. The word passthrough is important, because it means that the options are processed by put/4 specifically, rather than duplicated and handled by fetch/4.

Progressive complexity - explain the simplest usage first, adding complexity as the reader keeps reading.

I am fairly sure it is already written in this way. The only change I see that relates to this is:

Normally Cache stores the value returned by fallback.

The original documentation specifically points to :commit and :ignore first to push people to using those - there is an eventuality that the assumption of value = {:commit, value} will likely be phased out. It's inherited from earlier Cachex versions.

In general I'm leaning towards closing this PR without a merge. I could be convinced, particularly if @HansGlimmerfors wants to give their thoughts (in general), but as of right now I do not see the changes here as an improvement.

whitfin avatar Mar 25 '25 17:03 whitfin

In general I'm leaning towards closing this PR without a merge. I could be convinced, particularly if @HansGlimmerfors wants to give their thoughts (in general), but as of right now I do not see the changes here as an improvement.

I think @SergioInToronto's desire to help improve the documentation is great to see (but personally I prefer the original documentation). I would agree with @whitfin that these changes don't provide a strict improvement, so I think closing the PR could be the right way to go.

HansGlimmerfors avatar Mar 25 '25 19:03 HansGlimmerfors

Thanks for the feedback both! I'll try to convince you my PR is an improvement. I opened it after my own experience (and confusion) using Cachex.

First, I started building a mechanism exactly like Cachex.Services.Courier because I didn't know fetch/4 would handle it for me. Maybe there's documentation elsewhere but I didn't find it. I almost implemented it myself and missed such a great feature! I think adding 1 sentence about it would help people.

Cutting information only to make text smaller doesn't feel necessary to me.

Shorter without cutting information is my goal. Let me fix any mistakes.

An example is the :expire option to set an expiration from directly inside your fallback.

That's a good point - I've added it back. I will say I found the code example more helpful than explaining in words. Maybe the example with :expire should come sooner. But both is good too.

The original documentation specifically points to :commit and :ignore first to push people to using those - there is an eventuality that the assumption of value = {:commit, value} will likely be phased out. It's inherited from earlier Cachex versions.

Fair enough, I didn't know it was deprecated. I was explaining the simple usage first, but since it's discouraged I'll swap them.

As of Cachex v3.6, ...

I agree it's clear, but the documentation is already versioned. IMHO there's no need to mention specific versions this way.

image

I've pushed changes to address your feedback. I hope it's OK.

SergioInToronto avatar Mar 25 '25 20:03 SergioInToronto