You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Introduced getArgument method in LocalValue to handle various argument types including String, Number, Boolean, Instant, Map, List, Set, and RegExpValue.
Added execute method to RemoteScript to execute scripts with arguments, utilizing LocalValue.getArgument for argument handling.
Updated Script interface to include the execute method.
Added comprehensive tests for the execute method in WebScriptExecuteTest to verify handling of different argument types.
Changes walkthrough 📝
Relevant files
Enhancement
LocalValue.java
Add `getArgument` method to handle various argument types
Code Duplication The method getArgument contains repetitive code blocks for handling different types of arguments. Consider refactoring to reduce duplication and improve maintainability.
Exception Handling The method execute throws a generic WebDriverException without specific error handling or categorization. Consider throwing more specific exceptions or handling different error scenarios distinctly.
-throw new WebDriverException(- "Error while executing script: "- + ((EvaluateResultExceptionValue) result).getExceptionDetails().getText());+throw new WebDriverException(String.format("Error while executing script: %s", + ((EvaluateResultExceptionValue) result).getExceptionDetails().getText()));
Apply this suggestion
Suggestion importance[1-10]: 8
Why: Using String.format enhances readability and maintainability of the exception message construction, making the code cleaner and easier to understand.
8
Replace multiple if statements with a switch statement for type checking
Consider using a switch statement for the Number type check instead of multiple if statements. This will make the code more readable and maintainable.
-if (arg instanceof Integer || arg instanceof Long) {- localValue = numberValue(((Number) arg).longValue());-} else if (arg instanceof Double || arg instanceof Float) {- localValue = numberValue(((Number) arg).doubleValue());-} else if (arg instanceof BigInteger) {- localValue = bigIntValue(arg.toString());+switch (arg.getClass().getSimpleName()) {+ case "Integer":+ case "Long":+ localValue = numberValue(((Number) arg).longValue());+ break;+ case "Double":+ case "Float":+ localValue = numberValue(((Number) arg).doubleValue());+ break;+ case "BigInteger":+ localValue = bigIntValue(arg.toString());+ break;
}
Apply this suggestion
Suggestion importance[1-10]: 7
Why: The suggestion improves readability and maintainability by using a switch statement instead of multiple if statements. However, the improvement is minor as the original code is already clear.
7
Best practice
Use Optional to handle null values more gracefully
Consider using Java's Optional to handle potential null values gracefully instead of directly returning null.
Why: Using Optional can help handle potential null values more gracefully, but it introduces additional complexity and may not be necessary in this context.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Related to #13992
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Motivation and Context
Types of changes
Checklist
PR Type
Enhancement, Tests
Description
getArgumentmethod inLocalValueto handle various argument types including String, Number, Boolean, Instant, Map, List, Set, and RegExpValue.executemethod toRemoteScriptto execute scripts with arguments, utilizingLocalValue.getArgumentfor argument handling.Scriptinterface to include theexecutemethod.executemethod inWebScriptExecuteTestto verify handling of different argument types.Changes walkthrough 📝
LocalValue.java
Add `getArgument` method to handle various argument typesjava/src/org/openqa/selenium/bidi/script/LocalValue.java
getArgumentmethod to handle various argument types.Boolean, Instant, Map, List, Set, and RegExpValue.
RemoteScript.java
Add `execute` method to RemoteScript for script executionjava/src/org/openqa/selenium/remote/RemoteScript.java
executemethod to execute scripts with arguments.LocalValue.getArgumentfor argument handling.Script.java
Add `execute` method to Script interfacejava/src/org/openqa/selenium/remote/Script.java
executemethod signature to Script interface.WebScriptExecuteTest.java
Add tests for `execute` method with various argument typesjava/test/org/openqa/selenium/WebScriptExecuteTest.java
executemethod with various argument types.BigInt, arrays, sets, dates, maps, objects, and RegExp.