More APIs for Student Information #18
Conversation
Refactor (only_visible and only_active) to (visible_filter and active_filter)
shadinaif
force-pushed
the
shadinaif/more-apis
branch
4 times, most recently
from
4c47b38 to
b1ee08f
Compare
shadinaif
force-pushed
the
shadinaif/more-apis
branch
from
b1ee08f to
be58b87
Compare
Ensure the user is passed as User object and not its ID
get_block_structure_manager expects a string course key
ensure Grade is read as JSON
shadinaif
force-pushed
the
shadinaif/more-apis
branch
3 times, most recently
from
d0de299 to
41c54c6
Compare
|
Thanks @shadinaif! We'll deploy the tag and keep this pull request for review. @iamjazzar will review it today. |
| ) if callable(key_generator_or_name) else key_generator_or_name | ||
|
|
||
| except Exception as exc: # pylint: disable=broad-except | ||
| log.exception("cache_dict: error generating cache key: %s", exc) |
There was a problem hiding this comment.
It seems this code logs the error and continues. Would it be better to raise an exception here to halt execution and signal the issue?
There was a problem hiding this comment.
Since the code allows key_generator_or_name to be callable or str; it can fail because of a silly key generation mistake. I've tested my generators, but I mean if a new one is used without proper testing; I don't want it to block the execution of the APIs. An error message that will popup many times in the logs will be enough to draw the attention
I prefer keeping it passing without interrupting the API execution. This means we'll lose only the cache; not the entire response
What do you think @iamjazzar ?
I would also appreciate your feedback @OmarIthawi
| result = cache.get(cache_key) if cache_key else None | ||
| if result is None: | ||
| result = func(*args, **kwargs) | ||
| if cache_key and result and isinstance(result, dict): |
There was a problem hiding this comment.
This brings up an interesting point about caching empty dictionaries ({}). Caching the results of expensive functions (like a 200ms web query that can return empty results) can definitely improve performance.
However, it's worth considering the implications:
- Cache invalidation: If the function's result can change over time (e.g., the web query might return a value later), we need a way to invalidate the cache to ensure we fetch fresh data.
- Memory overhead: Even empty dictionaries consume some memory. If caching empties is frequent, the memory usage can add up.
There was a problem hiding this comment.
Good point @iamjazzar . I'll keep this in mind! currently, I'm using it for only two functions that are being used too many times with small result sizes --- empty dict is not possible for these two
shadinaif
force-pushed
the
shadinaif/more-apis
branch
from
41c54c6 to
f029f11
Compare
|
Thank you for the review @iamjazzar , please take a look now |
More APIs for Student Information