Skip to content
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

[java] refactor(remote/command): Merge overload's business logic #14469

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

nnnnoel
Copy link

@nnnnoel nnnnoel commented Sep 3, 2024

User description

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

Merge overload's business logic to make it easy to see at a glance

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement, configuration changes


Description

  • Refactored overloaded methods in DriverCommand to streamline business logic by delegating deprecated methods to their Duration counterparts.
  • Updated Java language level in java.iml from JDK 11 to JDK 17 to ensure compatibility with newer Java features.

Changes walkthrough 📝

Relevant files
Enhancement
DriverCommand.java
Refactor overloaded methods for session and timeout commands

java/src/org/openqa/selenium/remote/DriverCommand.java

  • Refactored overloaded methods for NEW_SESSION and timeout settings.
  • Deprecated methods now delegate to their Duration counterparts.
  • Simplified method calls by removing redundant code.
  • +4/-8     
    Configuration changes
    java.iml
    Update Java language level to JDK 17                                         

    java/java.iml

    • Updated Java language level from JDK 11 to JDK 17.
    +1/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @CLAassistant
    Copy link

    CLAassistant commented Sep 3, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Regression
    The removal of Require.nonNull("Capabilities", capabilities) check in the NEW_SESSION method might lead to null pointer exceptions if null capabilities are passed.

    Language Level Change
    The LANGUAGE_LEVEL has been changed from JDK_11 to JDK_17, which might affect compatibility with older Java versions.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Sep 3, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add a null check for the input parameter to prevent potential null pointer exceptions
    Suggestion Impact:A null check for the capabilities parameter was added using Require.nonNull to prevent potential NullPointerException.

    code diff:

    +    Require.nonNull("Capabilities", capabilities);

    Consider adding a null check for the capabilities parameter before calling
    singleton() to prevent potential NullPointerException.

    java/src/org/openqa/selenium/remote/DriverCommand.java [165-166]

     static CommandPayload NEW_SESSION(Capabilities capabilities) {
    +  Require.nonNull("Capabilities", capabilities);
       return NEW_SESSION(singleton(capabilities));
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a null check for the capabilities parameter is crucial to prevent potential NullPointerException, which enhances the robustness of the code.

    9
    Maintainability
    Add a comment to explain the rationale behind a significant version upgrade

    Consider adding a comment explaining the reason for upgrading the LANGUAGE_LEVEL
    from JDK_11 to JDK_17, as this change might have implications for the project's
    compatibility and requirements.

    java/java.iml [6]

    +<!-- Upgraded to JDK 17 for improved performance and new language features -->
     <component name="NewModuleRootManager" LANGUAGE_LEVEL="JDK_17">
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a comment to explain the upgrade to JDK 17 is beneficial for maintainability, as it provides context for future developers about the project's compatibility and requirements.

    7
    Enhancement
    Use a more readable time unit conversion method for improved code clarity

    Consider using Duration.ofSeconds() instead of Duration.ofMillis() for better
    readability when converting from TimeUnit to Duration.

    java/src/org/openqa/selenium/remote/DriverCommand.java [347-348]

     static CommandPayload SET_IMPLICIT_WAIT_TIMEOUT(long time, TimeUnit unit) {
    -  return SET_IMPLICIT_WAIT_TIMEOUT(Duration.ofMillis(unit.toMillis(time)));
    +  return SET_IMPLICIT_WAIT_TIMEOUT(Duration.ofSeconds(unit.toSeconds(time)));
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: While using Duration.ofSeconds() can improve readability, it is a minor enhancement and does not significantly impact functionality or performance.

    5

    @nnnnoel nnnnoel changed the title refactor(remote/command): Merge overload's business logic [java] refactor(remote/command): Merge overload's business logic Sep 3, 2024
    Copy link
    Member

    @shs96c shs96c left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you for the PR. It looks great, but please revert the change to java.iml

    java/java.iml Outdated
    @@ -3,7 +3,7 @@
    <component name="ModuleRunConfigurationManager">
    <shared />
    </component>
    <component name="NewModuleRootManager" LANGUAGE_LEVEL="JDK_11">
    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We still target Java 11

    Copy link
    Member

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Since we use the Bazel IntelliJ plugin, I think we can actually delete this file entirely. I'll do some checks.

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @shs96c Thanks to review, and i'd revert that line to JDK_11.

    But selenium's ci runs via Java 17.
    So It in order not to get confused, we can considor to manage language level on workspace root. (not only java. maybe like nvmrc in node.js projects?)

    @nnnnoel
    Copy link
    Author

    nnnnoel commented Sep 27, 2024

    @shs96c I applied your review. Please can you see the submitted commit?

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants