refactor: inject url path parts instead of endpoints #315
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
@schloerke - I applied more of your feedback, which helped reduce the complexity of endpoint creation. I think this is in the right direction. You can ignore the docstrings for now. I'll mark the PR as ready for review once I have those fixed. |
tdstein
force-pushed
the
tdstein/jobs-endpoint-injection
branch
from
902d271 to
f57340d
Compare
tdstein
force-pushed
the
tdstein/jobs-endpoint-injection
branch
from
8b9f08a to
72b62ac
Compare
Co-authored-by: Barret Schloerke <[email protected]>
…endpoint-injection
|
@schloerke / @toph-allen—This is ready for final review. I believe all of the points we discussed have been addressed. I am pretty happy with the implementation now. Thanks for all of your thoughts and attention to detail! |
| endpoint = self._ctx.url + self._path + uid | ||
| response = self._ctx.session.get(endpoint) | ||
| result = response.json() | ||
| return self._to_instance(result) |
There was a problem hiding this comment.
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.
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 to find() to use the cached values, behaving differently.
There was a problem hiding this comment.
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 by find 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.
src/posit/connect/resources.py
Outdated
| if self.cached(): | ||
| conditions = {self._uid: uid} | ||
| result = self.find_by(**conditions) | ||
| else: | ||
| endpoint = self._endpoint + uid | ||
| response = self._ctx.session.get(endpoint) | ||
| result = response.json() | ||
| result = self._create_instance(**result) | ||
|
|
||
| if not result: | ||
| raise ValueError(f"Failed to find instance where {self._uid} is '{uid}'") | ||
| if result: | ||
| return result | ||
|
|
||
| return result | ||
| endpoint = self._ctx.url + self._path + uid | ||
| response = self._ctx.session.get(endpoint) | ||
| result = response.json() | ||
| return self._to_instance(result) |
There was a problem hiding this comment.
The whole function body could be something like...
conditions = {self._uid: uid}
result = self.find_by(**conditions)
if result is not None:
return result
raise ValueError(f"Object `\{ \"{self._uid}\": \"{ uid }\" \}` could not be found")(untested)
There was a problem hiding this comment.
looks good to me, pending resolving @schloerke’s comments :)
Refactors path building responsibilities to the creating action, eliminating a ton of complexity along the way.