-
Notifications
You must be signed in to change notification settings - Fork 110
Clarify docs and examples for fetch/4 #404
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
base: main
Are you sure you want to change the base?
Clarify docs and examples for fetch/4 #404
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good initiative to improve the documentation, but maybe some minor tweaks should still be made? :)
(Note: I just happened to have a look at the open PR and had some comments, whitfin should review this for it to be merged)
lib/cachex.ex
Outdated
Cachex will store any value `fallback` returns. If you want more | ||
control, return a Tuple like `{:commit, value}` or `{:ignore, value}`. | ||
Passing `:ignore` instructs Cachex to not store the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is less clear than the original documentation. First saying "Cachex will store any value fallback
returns" and then saying "Passing :ignore
instructs Cachex to not store the value" are a bit contradictory.
The original documentation instead explained the function of :commit
and :ignore
, before saying that the fallback is :commit
if none is provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - I've addressed the contradictory statement. But I didn't change it back entirely. Let me know if this sounds better or if I should still revert the whole thing.
lib/cachex.ex
Outdated
If a fallback function has an arity of 1, the requested entry key | ||
will be passed through to allow for contextual computation. If a | ||
function has an arity of 0, it will be executed without arguments. | ||
`fallback` optionally receives `key` as an argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`fallback` optionally receives `key` as an argument. | |
The arity of `fallback` can be either 0 or 1. If the arity is 1, then the requested key will be passed as an argument to `fallback`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- IMHO my sentence communicates the same information in fewer words, and the examples make this clear too.
- It's more elixir-y to talk about "arity". I think Elixir is fantastic but I don't like requiring new developers understand all the jargon before they can write useful Elixir code. There's no need to mention arity here - optional arguments is understood regardless if the reader is an expert or new to Elixir.
iex> Cachex.fetch(:my_cache, "key", fn -> "value" end) | ||
{ :commit, "value" } | ||
iex> Cachex.fetch(:my_cache, "key", fn -> "value" end) | ||
{ :ok, "value" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted show the return values in these two cases. I was surprised at first usage that :commit and :ok cases both exist.
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.
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.
Cutting information only to make text smaller doesn't feel necessary to me. For example consider this:
Which was replaced with:
I am fairly sure it is already written in this way. The only change I see that relates to this is:
The original documentation specifically points to 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. |
7dd9fa7
to
0d609f8
Compare
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
Shorter without cutting information is my goal. Let me fix any mistakes.
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
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.
I agree it's clear, but the documentation is already versioned. IMHO there's no need to mention specific versions this way. I've pushed changes to address your feedback. I hope it's OK. |
fetch/4