Skip to content

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 5, 2025

User description

🔗 Related Issues

fixes #14291

💥 What does this PR do?

🔧 Implementation Notes

This pull request introduces changes to the SafariDriverService class and its associated Bazel build file to improve nullability handling and enhance type safety using @Nullable annotations. These changes aim to make the codebase more robust and compatible with tools that rely on nullability annotations.

Nullability Enhancements in SafariDriverService:

  • Added @Nullable annotations to method parameters: Updated multiple constructors and methods, including createDriverService, to mark parameters like File, Duration, List<String>, and Map<String, String> as nullable, improving clarity and type safety. ([[1]](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-5ee8455f18e8d287ed7899d66b590a2e6e13197ef14f9a5213e830ff32d0d656L67-R72), [[2]](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-5ee8455f18e8d287ed7899d66b590a2e6e13197ef14f9a5213e830ff32d0d656L172-R177))
  • Modified class fields and methods: Added @Nullable annotations to fields such as diagnose and methods like withLogging and withLogFile to indicate optional values. ([[1]](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-5ee8455f18e8d287ed7899d66b590a2e6e13197ef14f9a5213e830ff32d0d656L129-R130), [[2]](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-5ee8455f18e8d287ed7899d66b590a2e6e13197ef14f9a5213e830ff32d0d656L142-R149))

Build File Update:

  • Added @maven//:org_jspecify_jspecify dependency: Updated the Bazel build file to include the org.jspecify.jspecify library, enabling the use of @Nullable annotations in the project. ([java/src/org/openqa/selenium/safari/BUILD.bazelR17](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-0b0f5cc1965c89ca728657da296b7183ad9abed4555a906b6846798c506130a8R17))

Import Statement Update:

  • Imported @Nullable annotation: Added org.jspecify.annotations.Nullable to the imports in SafariDriverService.java to support nullability annotations. ([java/src/org/openqa/selenium/safari/SafariDriverService.javaR35](https://github.com/SeleniumHQ/selenium/pull/16000/files#diff-5ee8455f18e8d287ed7899d66b590a2e6e13197ef14f9a5213e830ff32d0d656R35))

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add @Nullable annotations to SafariDriverService parameters

  • Enhance type safety with JSpecify nullability annotations

  • Update Bazel build configuration for JSpecify dependency


Changes diagram

flowchart LR
  A["SafariDriverService"] --> B["Add @Nullable annotations"]
  B --> C["Constructor parameters"]
  B --> D["Builder methods"]
  B --> E["Class fields"]
  F["BUILD.bazel"] --> G["Add JSpecify dependency"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
SafariDriverService.java
Add nullable annotations to parameters and fields               

java/src/org/openqa/selenium/safari/SafariDriverService.java

  • Import @Nullable annotation from JSpecify
  • Add @Nullable to constructor parameters (File, Duration, List, Map)
  • Add @Nullable to Builder class field diagnose
  • Add @Nullable to Builder methods withLogging and withLogFile
  • +13/-8   
    Dependencies
    BUILD.bazel
    Add JSpecify dependency to build configuration                     

    java/src/org/openqa/selenium/safari/BUILD.bazel

    • Add JSpecify dependency to Maven coordinates
    +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-java Java Bindings B-build Includes scripting, bazel and CI integrations labels Jul 5, 2025
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Annotation Consistency

    The diagnose field is annotated as @Nullable Boolean but the constructor and method parameters use @Nullable with primitive wrapper types. Verify that all nullable annotations are consistently applied and that the existing code properly handles null values for these parameters.

    private @Nullable Boolean diagnose;

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Remove misleading nullable annotation

    The method throws an exception regardless of the parameter value, making the
    @Nullable annotation misleading. Consider removing the annotation since the
    parameter is never actually used in the method body.

    java/src/org/openqa/selenium/safari/SafariDriverService.java [149-153]

    -public Builder withLogFile(@Nullable File logFile) {
    +public Builder withLogFile(File logFile) {
       throw new WebDriverException(
           "Can not set log location for Safari; use withLogging(true) and locate log in"
               + " ~/Library/Logs/com.apple.WebDriver/");
     }
    • Apply / Chat
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies that the @Nullable annotation on the logFile parameter is misleading because the method unconditionally throws an exception, making the parameter unused.

    Low
    • Update

    @diemol diemol merged commit e2958fd into SeleniumHQ:trunk Jul 7, 2025
    32 of 33 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-build Includes scripting, bazel and CI integrations C-java Java Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: JSpecify Nullness annotations for Java
    3 participants