Skip to content

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Sep 14, 2024

User description

Description

Bazel understands NET8_0, but doesn't understand NET8_0_OR_GREATER.

Motivation and Context

We want to use pre-processor directives.

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

Bug fix, Enhancement


Description

  • Added the NET8_0_OR_GREATER pre-processor directive in the Bazel build configuration to ensure compatibility with Bazel's understanding of .NET versioning.
  • This change acts as a workaround for Bazel's limitation in recognizing NET8_0_OR_GREATER.

Changes walkthrough 📝

Relevant files
Enhancement
BUILD.bazel
Add pre-processor directive for .NET version compatibility

dotnet/src/webdriver/BUILD.bazel

  • Added NET8_0_OR_GREATER to the defines section.
  • Ensures compatibility with Bazel's understanding of pre-processor
    directives.
  • +3/-0     

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

    @qodo-merge-pro qodo-merge-pro bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue Review effort [1-5]: 1 labels Sep 14, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Compatibility Concern
    The added pre-processor directive may not be recognized by older .NET versions, potentially causing compatibility issues.

    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 14, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a more specific .NET version define for better clarity and version control

    Consider using a more specific define for NET8_0 instead of NET8_0_OR_GREATER. This
    will make it clearer that the code is specifically targeting .NET 8.0 and not any
    future versions.

    dotnet/src/webdriver/BUILD.bazel [84-86]

     defines = [
    -    "NET8_0_OR_GREATER"
    +    "NET8_0"
     ],
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a more specific define for .NET 8.0 instead of NET8_0_OR_GREATER can improve clarity and version control. However, it may reduce flexibility for future versions, which could be a consideration depending on the project's needs.

    5

    @nvborisenko nvborisenko merged commit dd50e28 into SeleniumHQ:trunk Sep 14, 2024
    9 of 10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-directive branch September 26, 2024 11:17
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    * [dotnet] Workaround using pre-processor directive
    
    * Fix formatting?
    
    * Fix formatting
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 1

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    1 participant