Skip to content

[dotnet] Improve dependency update developer experience #15086

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 15, 2025
Merged

[dotnet] Improve dependency update developer experience #15086

merged 2 commits into from
Jan 15, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 14, 2025 •
edited by qodo-merge-pro bot
Loading

User description

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

This change fixes the update dependency command when running on Windows.

Motivation and Context

This command does not work on Windows at the moment, due to a double-quote issue

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

Description

  • Fixed a double-quote issue in update-deps.sh script.

  • Improved dependency update command compatibility on Windows.

Changes walkthrough 📝

Relevant files
Bug fix
update-deps.sh
Resolve double-quote issue in `update-deps.sh`                     

dotnet/update-deps.sh

  • Fixed a double-quote issue in the bazel run command.
  • Improved script compatibility for Windows environments.
    		• +1/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Path Validation

    Verify that the path construction with quotes works correctly across different Windows environments and shell configurations

    bazel run @rules_dotnet//tools/paket2bazel:paket2bazel -- --dependencies-file "$(pwd)/paket.dependencies" --output-folder "$(pwd)"

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    Category Suggestion                                                                                                                                    Score
    General
    Replace shell command substitution with environment variable for more reliable path handling across platforms

    Use POSIX-compliant path handling by replacing $(pwd) with "$PWD". The $PWD
    environment variable is more reliable across different shells and platforms.

    dotnet/update-deps.sh [8]

    -bazel run @rules_dotnet//tools/paket2bazel:paket2bazel -- --dependencies-file "$(pwd)/paket.dependencies" --output-folder "$(pwd)"
+bazel run @rules_dotnet//tools/paket2bazel:paket2bazel -- --dependencies-file "$PWD/paket.dependencies" --output-folder "$PWD"
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using $PWD is indeed more POSIX-compliant and slightly more efficient than $(pwd), making the script more portable across different shells and platforms. However, both approaches are commonly used and work reliably in most scenarios.

    		 6

    @RenderMichael RenderMichael changed the title [dotnet] Improve dependency update experience on Windows [dotnet] Improve dependency update developer experience Jan 14, 2025
    @RenderMichael RenderMichael merged commit cc814a8 into SeleniumHQ:trunk Jan 15, 2025
    10 checks passed
    @RenderMichael RenderMichael deleted the update branch January 15, 2025 05:43
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    @RenderMichael @sandeepsuryaprasad
    [dotnet] Improve dependency update developer experience (SeleniumHQ#1…
    7a7782d 
    …5086)

[dotnet] Improve dependency update experience on Windows
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    @RenderMichael @gryznar
    [dotnet] Improve dependency update developer experience (SeleniumHQ#1…
    30d0309 
    …5086)

[dotnet] Improve dependency update experience on Windows
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @nvborisenko nvborisenko nvborisenko approved these changes

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 1
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @RenderMichael @nvborisenko