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

EdgeOptions.useWebView to return "this" #14157

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

vlad8x8
Copy link
Contributor

@vlad8x8 vlad8x8 commented Jun 19, 2024

User description

All ChromiumOptions methods return this
This commit makes EdgeOptions.useWebView behave the same as other Options methods

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

All ChromiumOptions methods return this
This commit makes EdgeOptions.useWebView behave the same as other Options methods

Motivation and Context

All ChromiumOptions methods return this
This commit makes EdgeOptions.useWebView behave the same as other Options methods

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


Description

  • Modified the useWebView method in EdgeOptions class to return the EdgeOptions instance (this) instead of void.
  • Ensures consistency with other ChromiumOptions methods that return this.

Changes walkthrough 📝

Relevant files
Enhancement
EdgeOptions.java
Modify `useWebView` method to return `EdgeOptions` instance

java/src/org/openqa/selenium/edge/EdgeOptions.java

  • Modified useWebView method to return EdgeOptions instead of void.
  • Added a return statement to return this in the useWebView method.
  • +2/-1     

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

    All ChromiumOptions methods return `this`
    This commit makes EdgeOptions.useWebView behave the same as other Options methods
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 1
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review None

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add a method description to explain the purpose and behavior of the useWebView method

    Add a method description to explain the purpose and behavior of the useWebView method for
    better code readability.

    java/src/org/openqa/selenium/edge/EdgeOptions.java [65-69]

    +/**
    + * Enables or disables the use of WebView2.
    + *
    + * @param enable boolean flag to enable or disable the 'webview2' usage
    + * @return the current EdgeOptions instance
    + */
     public EdgeOptions useWebView(boolean enable) {
       String browserName = enable ? WEBVIEW2_BROWSER_NAME : EDGE.browserName();
       setCapability(CapabilityType.BROWSER_NAME, browserName);
       return this;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a method description improves code readability and maintainability, which is a good practice, especially for public APIs.

    7
    Possible issue
    Ensure that the browser name is not null to prevent potential issues when setting capabilities

    Ensure that the WEBVIEW2_BROWSER_NAME and EDGE.browserName() are not null to prevent
    potential issues when setting capabilities.

    java/src/org/openqa/selenium/edge/EdgeOptions.java [65-69]

     public EdgeOptions useWebView(boolean enable) {
       String browserName = enable ? WEBVIEW2_BROWSER_NAME : EDGE.browserName();
    +  if (browserName == null) {
    +    throw new IllegalStateException("Browser name cannot be null");
    +  }
       setCapability(CapabilityType.BROWSER_NAME, browserName);
       return this;
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While ensuring non-null values is generally good, the suggestion lacks context on whether WEBVIEW2_BROWSER_NAME or EDGE.browserName() could realistically be null, which affects the necessity of this check.

    4

    Copy link
    Member

    @titusfortner titusfortner left a comment

    Choose a reason for hiding this comment

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

    Good catch. Can you add a return statement to the javadoc there as well? Thanks.

    @titusfortner titusfortner merged commit aca918b into SeleniumHQ:trunk Jun 20, 2024
    33 of 36 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    * EdgeOptions.useWebView to return this
    
    All ChromiumOptions methods return `this`
    This commit makes EdgeOptions.useWebView behave the same as other Options methods
    
    * update javadoc
    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.

    2 participants