chore: Enhanced missing local storage error #427
Conversation
github-actions
bot
added
t-tooling
Pijukatel
requested review from
vdusek and
janbuchar
and removed request for
vdusek
There was a problem hiding this comment.
I have a few questions about the decorator.
I'm also wondering if we're not using a sledgehammer to crack a nut.
From the issue description:
Yeah, it was reported by me.
I did some tests and to me it seems to be working as expected.
Cool 👍 As discussed in the Slack thread, there is no actual bug, it's just that mixing remote and local development makes things really confusing. So updating the documentation makes sense.
On top of that, how about updating the error message? If:
I access storage by ID, and
I'm in local environment, and
the storage does not exist, then
show an error mentioning that the issue might be that the storage is remote?
So this should be really just about updating the error message. I would rather do it directly rather than introduce this decorator-obscuration magic.
It seems like this should be only about updating the error message. Maybe I would rather do it directly.
| ) -> Any: | ||
| try: | ||
| return await function(self=self, id=id, name=name, force_cloud=force_cloud) | ||
| except Exception as e: |
There was a problem hiding this comment.
Do we really want to catch all exceptions?
| e.args = ( | ||
| f'{e.args[0]} (If you are trying to retrieve a remote storage, use `force_cloud=True` argument.)', | ||
| ) |
There was a problem hiding this comment.
Couldn't e.args contain more than one item? If so, am I correct that we are discarding them?
| TFun = TypeVar('TFun', bound=Callable[..., Any]) | ||
|
|
||
|
|
||
| def _add_local_storage_error_hint(function: TFun) -> TFun: |
There was a problem hiding this comment.
Couldn't we move the decorator to a dedicated private module, perhaps apify._utils?
| ) | ||
| raise | ||
|
|
||
| return cast(TFun, wrapper) |
There was a problem hiding this comment.
Do we really need the cast? Doesn't functools.wraps already take care of preserving the function's type?
I am not sure this is even necessary. I raised the question in the slack, but no one responded there, so this is just the same question more aloud :-) This error originates from I am not saying that inside that decorator it is nice, but I think in general it is correct approach to detach such additional error message enhancement (If needed at all?!) to decorator so that it is not polluting the main function logic. |
Enhanced missing local storage error
Closes: #412