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] JSpecify annotations for wrappers #14396

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

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Aug 14, 2024

User description

Description

In this PR I'm adding nullness annotations for interfaces:

  • WrapsDriver
  • WrapsElement

None of the methods in these wrappers can return a null value

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

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, documentation


Description

  • Added JSpecify @NullMarked annotations to the WrapsDriver and WrapsElement interfaces.
  • These annotations help developers identify potential nullability issues, enhancing code safety and preventing NullPointerExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
WrapsDriver.java
Add JSpecify nullness annotations to WrapsDriver                 

java/src/org/openqa/selenium/WrapsDriver.java

  • Added @NullMarked annotation to the WrapsDriver interface.
  • Imported org.jspecify.annotations.NullMarked.
  • +3/-0     
    WrapsElement.java
    Add JSpecify nullness annotations to WrapsElement               

    java/src/org/openqa/selenium/WrapsElement.java

  • Added @NullMarked annotation to the WrapsElement interface.
  • Imported org.jspecify.annotations.NullMarked.
  • +3/-0     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add a brief explanation for the NullMarked annotation in the JavaDoc

    Consider adding a brief JavaDoc comment to explain the purpose of the @NullMarked
    annotation, as it might not be immediately clear to all developers.

    java/src/org/openqa/selenium/WrapsDriver.java [20-26]

     import org.jspecify.annotations.NullMarked;
     
     /**
      * This interface indicates that the implementing class knows about the driver that contains it and
      * can export it.
    + *
    + * The {@code @NullMarked} annotation indicates that null-checking is enabled for this interface.
      */
     @NullMarked
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Providing a JavaDoc comment explaining the @NullMarked annotation improves maintainability by making the code more understandable, especially for developers unfamiliar with this annotation.

    8
    Add a JavaDoc comment to the interface method explaining its purpose and return value

    Consider adding a brief JavaDoc comment to the getWrappedElement() method to explain
    its purpose and any potential null behavior.

    java/src/org/openqa/selenium/WrapsElement.java [23-27]

     @NullMarked
     @FunctionalInterface
     public interface WrapsElement {
    +  /**
    +   * Returns the underlying WebElement wrapped by this instance.
    +   *
    +   * @return The wrapped WebElement, which should not be null.
    +   */
       WebElement getWrappedElement();
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a JavaDoc comment enhances maintainability by clearly documenting the method's purpose and expected behavior, which is valuable for developers interacting with this interface.

    8
    Enhancement
    Add a nullable annotation to the return type of the interface method

    Consider adding a @Nullable annotation to the return type of the method in the
    WrapsDriver interface to explicitly indicate whether the returned driver can be null
    or not.

    java/src/org/openqa/selenium/WrapsDriver.java [26-30]

     @NullMarked
     @FunctionalInterface
     public interface WrapsDriver {
       /**
        * @return The driver that contains this element.
    +   */
    +  @Nullable
    +  WebDriver getWrappedDriver();
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a @Nullable annotation could improve code clarity by explicitly indicating the nullability of the return type, which is beneficial for developers using this interface.

    7
    Add a non-null annotation to the return type of the interface method

    Consider adding a @Nullable or @NonNull annotation to the return type of the
    getWrappedElement() method to explicitly indicate whether the returned element can
    be null or not.

    java/src/org/openqa/selenium/WrapsElement.java [23-27]

     @NullMarked
     @FunctionalInterface
     public interface WrapsElement {
    +  @NonNull
       WebElement getWrappedElement();
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a @NonNull annotation would enhance code clarity by explicitly stating that the return value should not be null, which is useful for developers using this interface.

    7

    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