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

[dotnet] Workaround using pre-processor directive #14499

Merged
merged 3 commits into from
Sep 14, 2024

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

    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

    codiumai-pr-agent-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
    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.

    1 participant