-
-
Notifications
You must be signed in to change notification settings - Fork 304
refactor(tab): separate execute_script concerns #258
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
...rehensive options
Codecov Report
✅ All modified and coverable lines are covered by tests.
📢 Thoughts on this report? Let us know!
Last commit refactors JavaScript execution by moving execute_element_script functionality from the Tab class to the WebElement class for better encapsulation.
I think this is too much. We can keep something with a minimal useful API, and for advanced users, they can execute the scripts directly.
For instance, something like this would be better:
@overload async def execute_script(self, script: str, await_promise: bool = True, return_by_value: bool = True) -> EvaluateResponse: ... @overload async def execute_script(self, script: str, element: WebElement, await_promise: bool = True, return_by_value: bool = True) -> CallFunctionOnResponse: ...
I think this is too much. We can keep something with a minimal useful API, and for advanced users, they can execute the scripts directly.
For instance, something like this would be better:
@overload async def execute_script(self, script: str, await_promise: bool = True, return_by_value: bool = True) -> EvaluateResponse: ... @overload async def execute_script(self, script: str, element: WebElement, await_promise: bool = True, return_by_value: bool = True) -> CallFunctionOnResponse: ...
Oh, I completely forgot about this PR due to the delayed response, and you probably closed it because of inactivity. If that’s the case, it would’ve been better to just ping me instead of closing it. I’ve just submitted the latest commit with the requested changes, so please consider reopening this PR. Also, let me explain the situation a little better.
The current code from the PR uses explicit methods. Tab.execute_script is clearly intended for page-level JavaScript execution, while WebElement.execute_script is meant for element-specific operations. Both rely on different runtime functions and return different data types, so combining them would blur their distinct use cases and create confusion. Since the WebElement class already implements this function, duplicating it in WebElement isn’t necessary.
Tab.execute_script(inpydoll/browser/tab.py):
async def execute_script(
self,
script: str,
*,
return_by_value: Optional[bool] = None,
await_promise: Optional[bool] = None,
) -> EvaluateResponse:
WebElement.execute_script(inpydoll/elements/web_element.py):
async def execute_script(
self,
script: str,
*,
return_by_value: Optional[bool] = None,
await_promise: Optional[bool] = None,
) -> CallFunctionOnResponse:
Honestly, it’d be great to keep the other arguments, as having the flexibility to tweak these parameters could be useful in some situations.
Hello @3xp0rt, thanks for the feedback. We have to keep the API consistent, otherwise it'll be a breaking change. I understand your point of view, and that makes sense. My idea would be: if the user pass an WebElement to the Tab.execute_script method, we can show a warning message informing that this is deprecated, and to use WebElement.execute_script instead;
For now, we can keep all the arguments, but we should preserve in just one method for clarity. Lets keep just execute_script method, and use overload for correct typing inference:
@overload # this is the signature without passing an WebElement as argument; async def execute_script( self, script: str, *, object_group: Optional[str] = None, include_command_line_api: Optional[bool] = None, silent: Optional[bool] = None, context_id: Optional[int] = None, return_by_value: Optional[bool] = None, generate_preview: Optional[bool] = None, user_gesture: Optional[bool] = None, await_promise: Optional[bool] = None, throw_on_side_effect: Optional[bool] = None, timeout: Optional[float] = None, disable_breaks: Optional[bool] = None, repl_mode: Optional[bool] = None, allow_unsafe_eval_blocked_by_csp: Optional[bool] = None, unique_context_id: Optional[str] = None, serialization_options: Optional[SerializationOptions] = None, ) -> EvaluateResponse: @overload async def execute_element( # this is the signature passing an WebElement as arg self, script: str, element: WebElement, *, arguments: Optional[list[CallArgument]] = None, silent: Optional[bool] = None, return_by_value: Optional[bool] = None, generate_preview: Optional[bool] = None, user_gesture: Optional[bool] = None, await_promise: Optional[bool] = None, execution_context_id: Optional[int] = None, object_group: Optional[str] = None, throw_on_side_effect: Optional[bool] = None, unique_context_id: Optional[str] = None, serialization_options: Optional[SerializationOptions] = None, ) -> CallFunctionOnResponse: async def execute_element( # this is the actual implementation self, script: str, element: Optional[WebElement] = None, *, arguments: Optional[list[CallArgument]] = None, silent: Optional[bool] = None, return_by_value: Optional[bool] = None, generate_preview: Optional[bool] = None, user_gesture: Optional[bool] = None, await_promise: Optional[bool] = None, execution_context_id: Optional[int] = None, object_group: Optional[str] = None, throw_on_side_effect: Optional[bool] = None, unique_context_id: Optional[str] = None, serialization_options: Optional[SerializationOptions] = None, ) -> Union[EvaluateResponse, CallFunctionOnResponse]:
What do you think? Just make sure to document every param properly
Refactoring Pull Request
Refactoring Scope
The Tab class's
execute_scriptmethod inpydoll/browser/tab.py, related tests and documentation.Related Issue(s)
Addresses user feedback from GitHub Discussion #108 regarding async script execution capabilities
Description
Refactor the
execute_scriptmethod by separating concerns to improve the API design. The new implementation should support asynchronous execution usingawait_promise=Trueandreturn_by_value=True, provide additional options for flexible usage in code. The original method handled both general script execution and element-specific execution, which has been split into two focused methods:execute_script: Enhanced with comprehensive runtime options for general script executionexecute_element_script: New method specifically for executing scripts with element contextMotivation
Support for asynchronous code.
Before / After
Before
After
Performance Impact
Technical Debt
This refactoring addresses several technical debt issues:
API Changes
Testing Strategy
Testing Checklist
Risks and Mitigations
execute_script(script, element)signatureChecklist before requesting a review
poetry run task lintand fixed any issuespoetry run task testand all tests pass