-
Notifications
You must be signed in to change notification settings - Fork 18
chore: utilize ensure_context decorator for Actor
#400
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
Conversation
janbuchar
left a comment
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'm honestly not sure about this. It reduces some duplication, yes, but the error message will be pretty cryptic. It is fine for an internal helper in Crawlee, but here, it's very much user-facing. Maybe we could make a wrapper for the decorator?
|
Error message: Whole traceback: Is it that bad? We can wrap it of course. |
Well, leaking the existence of |
Pijukatel
left a comment
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 like this decorator, maybe we have to have custom error message in this case, but I think it makes the code more readable.
|
Yep, I will try to update the decorator to have a better error message there. |
janbuchar
left a comment
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.
Please wrap that error message 🙂
|
Clousing, since we moved the flag(s) to the class level, and I'm afraid this means we can't utilize |
Utilize
ensure_contextdecorator from Crawlee rather than callingraise_if_...method in the Actor class.