Skip to content

[rust] Include SM flag --selenium-version to specify the Selenium version (#15754) #15755

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

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

bonigarcia
Copy link
Member

@bonigarcia bonigarcia commented May 19, 2025

User description

🔗 Related Issues

This PR implements the Rust side of #15754.

💥 What does this PR do?

It is implements a new CLI flag in Selenium Manager called --selenium-version to specify the Selenium version to be reported to Plausible.

🔧 Implementation Notes

The bindings should use this flag to specify its own version.

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add --selenium-version CLI flag to Selenium Manager

  • Pass Selenium version from CLI to Plausible reporting

  • Remove internal beta label logic for version reporting


Changes walkthrough 📝

Relevant files
Enhancement
main.rs
Add and use --selenium-version CLI flag in main logic       

rust/src/main.rs

  • Add --selenium-version CLI argument to CLI struct
  • Pass CLI-provided Selenium version to SeleniumManager
  • Remove internal logic for stripping beta label from version
  • Update usage to directly use CLI-provided version for reporting
  • +6/-4     
    Cleanup
    lib.rs
    Remove unused beta label constant                                               

    rust/src/lib.rs

    • Remove unused SM_BETA_LABEL constant
    +0/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @bonigarcia bonigarcia added the C-rust Rust code is mostly Selenium Manager label May 19, 2025
    @bonigarcia bonigarcia moved this to In Progress in Selenium Manager May 19, 2025
    @selenium-ci selenium-ci added the B-manager Selenium Manager label May 19, 2025
    Copy link
    Contributor

    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

    Default Value

    The new CLI parameter --selenium-version has an empty string as default value. Consider if this is the intended behavior or if it should default to the current Selenium Manager version.

    #[clap(long, value_parser, default_value = "")]
    selenium_version: String,
    Missing Documentation

    The PR removes the internal version detection logic but doesn't document that bindings are now expected to provide their version. This change in responsibility should be clearly documented.

    selenium_manager.set_language_binding(cli.language_binding.unwrap_or_default());
    selenium_manager.set_selenium_version(cli.selenium_version);

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Improve default version value

    The selenium_version parameter has an empty string as default value, which might
    cause issues when analytics expect a valid version string. Consider using a more
    descriptive default value or adding validation logic to handle empty values.

    rust/src/main.rs [148-150]

     /// Selenium version (to be sent to plausible.io)
    -#[clap(long, value_parser, default_value = "")]
    +#[clap(long, value_parser, default_value = "unknown")]
     selenium_version: String,
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: Changing the default from an empty string to "unknown" for selenium_version improves clarity and may help downstream analytics, but it is a minor improvement and not critical to functionality.

    Low
    • More

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager Review effort 2/5
    Projects
    Status: In Progress
    Development

    Successfully merging this pull request may close these issues.

    2 participants