-
Notifications
You must be signed in to change notification settings - Fork 811
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
update pre-commit #1163
update pre-commit #1163
Conversation
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.
❌ Changes requested. Reviewed everything up to 010ed96 in 1 minute and 0 seconds
More details
- Looked at
54
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/webeye/actions/handler.py:1135
- Draft comment:
The conditionif isinstance(sibling_action_result, ActionSuccess):
should beif not isinstance(sibling_action_result, ActionSuccess):
to match the original logic of checking for non-successful actions. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_4rAAHGq4VXf0Y1ds
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
skyvern/webeye/actions/handler.py
Outdated
@@ -248,7 +248,7 @@ async def handle_action( | |||
handler = ActionHandler._handled_action_types[action.action_type] | |||
results = await handler(action, page, scraped_page, task, step) | |||
actions_result.extend(results) | |||
if not results or type(actions_result[-1]) != ActionSuccess: | |||
if not results or isinstance(actions_result[-1], ActionSuccess): |
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 condition if not results or isinstance(actions_result[-1], ActionSuccess):
seems incorrect. It should be if not results or not isinstance(actions_result[-1], ActionSuccess):
to match the original logic of checking for non-successful actions.
if not results or isinstance(actions_result[-1], ActionSuccess): | |
if not results or not isinstance(actions_result[-1], ActionSuccess): |
010ed96
to
9cd8605
Compare
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.
👍 Looks good to me! Incremental review on 9cd8605 in 18 seconds
More details
- Looked at
54
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern/webeye/actions/handler.py:251
- Draft comment:
The use ofisinstance()
for type checking is a good practice. It is more reliable and readable than usingtype()
. This change is correctly implemented here and infc_func()
. - Reason this comment was not posted:
Confidence changes required:0%
The use ofisinstance()
is a good practice for type checking, as it is more reliable and readable than usingtype()
. The changes made in the PR are correct and align with best practices.
Workflow ID: wflow_8NuzIITbYeZjcntf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Update pre-commit versions and replace
type()
withisinstance()
for type checking inhandler.py
.pre-commit-hooks
tov5.0.0
,ruff-pre-commit
tov0.7.3
, andmirrors-mypy
tov1.13.0
in.pre-commit-config.yaml
.type()
withisinstance()
for type checking inhandle_action()
andfc_func()
inhandler.py
.This description was created by for 9cd8605. It will automatically update as commits are pushed.