Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
refactor: inject url path parts instead of endpoints #315
refactor: inject url path parts instead of endpoints #315
Changes from 32 commits
0533f19
6b79912
279fcd6
e349870
533839b
1066ca3
437c515
a1ca377
82b9b7e
6b8126d
b64f3e7
f57340d
72b62ac
f1d6f42
dd74d60
fb52c83
bbbd6b4
bc2cfcb
9c3d6dd
9019386
4bfe3f8
a721b61
107ee85
a070f0a
d196271
d87cfe7
2add280
97d24f6
7eeb054
03accf8
77f8a38
ac488c7
b3ff1cd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Note that I removed the call to invalidate the cache that existed a few commits before. After some additional consideration, I concluded that invalidating the cache is an unwanted side effect.
I think there is still an argument for invalidating the cache or appending the instance to the cached list. But, I don't think we have a good enough understanding of the side effects to proceed with either implementation.
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.
Why not just call
.find_by()
all the time?If the cache exists,
._data
returns quickly. If not, it asks the server.Then both methods have the same quirks. (Where as
find()
will not alter the cache, but find_by will... causing a followup call tofind()
to use the cached values, behaving differently.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 in either situation, we end up with conflicting ideas.
If we always depend on
find_by
, we must fetch the entire collection before returning a single value, which would take significantly longer than a single GET request for the value.Today, I think the obvious solution would be to always call the HTTP GET method to get the value from the server. This will sometimes be slightly slower than an in-memory list scan. But in reality, it's going to be a negligible difference. The weird edge case with this solution is when another process creates the value fetched by GET after the
_cache
is set. In this situation, the value returned byfind
will exist on the server but not in the _cache. This would probably warrant a cache invalidation. But that would take extra time to compute and may not be consistent behavior across all endpoints.tl;dr - the speed up via
find_by
probably isn't worth the trouble. Classic over engineering.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.
The whole function body could be something like...
(untested)