Skip to content

Conversation

@titusfortner
Copy link
Member

💥 What does this PR do?

Updates Ruby lint configuration and fixes rubocop offenses to reduce inline disable comments:

  • Remove unnecessary exclusions from .rubocop.yml for files that now pass lint checks
  • Add Steepfile to BlockLength exclusion instead of inline disable comment
  • Add proper respond_to_missing? methods where method_missing is used (best practice in Ruby)
  • Rename single-letter parameters to descriptive names in Color.from_hsl
  • Remove duplicate test in driver_spec.rb
  • Move Style/MixinUsage exclusion to .rubocop.yml

🔧 Implementation Notes

The goal is to reduce the number of inline rubocop disable comments by either:

  1. Fixing the underlying code to comply with the rule
  2. Moving necessary exclusions to .rubocop.yml where they're more visible

💡 Additional Considerations

None - all changes verified by pre-push hook running rubocop and steep.

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-rb Ruby Bindings B-support Issue or PR related to support classes labels Jan 26, 2026
@qodo-code-review
Copy link
Contributor

User description

💥 What does this PR do?

Updates Ruby lint configuration and fixes rubocop offenses to reduce inline disable comments:

  • Remove unnecessary exclusions from .rubocop.yml for files that now pass lint checks
  • Add Steepfile to BlockLength exclusion instead of inline disable comment
  • Add proper respond_to_missing? methods where method_missing is used (best practice in Ruby)
  • Rename single-letter parameters to descriptive names in Color.from_hsl
  • Remove duplicate test in driver_spec.rb
  • Move Style/MixinUsage exclusion to .rubocop.yml

🔧 Implementation Notes

The goal is to reduce the number of inline rubocop disable comments by either:

  1. Fixing the underlying code to comply with the rule
  2. Moving necessary exclusions to .rubocop.yml where they're more visible

💡 Additional Considerations

None - all changes verified by pre-push hook running rubocop and steep.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Add respond_to_missing? methods to comply with Ruby best practices

  • Rename single-letter parameters to descriptive names in Color.from_hsl

  • Remove duplicate test case in driver_spec.rb

  • Clean up rubocop configuration by removing unnecessary exclusions

  • Move Style/MixinUsage exclusion to .rubocop.yml configuration


Diagram Walkthrough

flowchart LR
  A["Rubocop Offenses"] --> B["Add respond_to_missing?"]
  A --> C["Rename Parameters"]
  A --> D["Remove Duplicates"]
  B --> E["Cleaner Code"]
  C --> E
  D --> E
  F[".rubocop.yml"] --> G["Remove Exclusions"]
  F --> H["Add Steepfile"]
  F --> I["Add MixinUsage"]
  G --> J["Reduced Config"]
  H --> J
  I --> J
Loading

File Walkthrough

Relevant files
Enhancement
block_event_listener.rb
Add respond_to_missing? to BlockEventListener                       

rb/lib/selenium/webdriver/support/block_event_listener.rb

  • Remove Style/MissingRespondToMissing inline disable comment
  • Add respond_to_missing? method implementation
  • Method now complies with Ruby best practices for method_missing
+5/-1     
color.rb
Rename parameters for clarity in from_hsl                               

rb/lib/selenium/webdriver/support/color.rb

  • Rename single-letter parameters h, s, l, a to hue, sat, light, alpha
  • Remove Naming/MethodParameterName inline disable comment
  • Update all references to renamed parameters throughout method
+14/-14 
event_firing_bridge.rb
Add respond_to_missing? to EventFiringBridge                         

rb/lib/selenium/webdriver/support/event_firing_bridge.rb

  • Remove Style/MissingRespondToMissing inline disable comment
  • Add respond_to_missing? method with proper delegation
  • Method now complies with Ruby best practices for method_missing
+5/-1     
Cleanup
driver_spec.rb
Remove duplicate test case                                                             

rb/spec/integration/selenium/webdriver/driver_spec.rb

  • Remove duplicate test case "finds by class name"
  • Remove associated RSpec/RepeatedExample inline disable comments
  • Keep single test with cleaner implementation
+1/-7     
spec_helper.rb
Remove inline disable comment                                                       

rb/spec/integration/selenium/webdriver/spec_helper.rb

  • Remove Style/MixinUsage inline disable comment from include Selenium
  • Exclusion moved to .rubocop.yml for centralized configuration
+1/-1     
Steepfile
Remove inline rubocop disable comments                                     

rb/Steepfile

  • Remove rubocop:disable Metrics/BlockLength comment at start of file
  • Remove rubocop:enable Metrics/BlockLength comment at end of file
  • Add Steepfile to .rubocop.yml exclusions instead
+0/-2     
Configuration changes
.rubocop.yml
Consolidate and clean up rubocop exclusions                           

rb/.rubocop.yml

  • Remove exclusions for options.rb and capabilities.rb from
    Metrics/AbcSize
  • Add Steepfile to Metrics/BlockLength exclusions
  • Remove multiple unnecessary exclusions from Metrics/ClassLength,
    Metrics/CyclomaticComplexity, Metrics/MethodLength, and
    Metrics/PerceivedComplexity
  • Add new Style/MixinUsage section with exclusion for spec_helper.rb
+5/-12   

@selenium-ci
Copy link
Member

Thank you, @titusfortner for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 26, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jan 26, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix splat forwarding in method_missing

*Fix a syntax error in method_missing by naming the splat argument (e.g., args)
to correctly forward arguments to the callback.

rb/lib/selenium/webdriver/support/block_event_listener.rb [28-29]

-def method_missing(meth, *)
-  @callback.call(meth, *)
+def method_missing(meth, *args)
+  @callback.call(meth, *args)
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a syntax error in the PR where an anonymous splat * is used for argument forwarding, which is invalid. Fixing this is critical for the code to run.

High
  • Update

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates Ruby lint configuration and fixes RuboCop offenses with the goal of removing inline disable comments and aligning code with Ruby best practices around method_missing.

Changes:

  • Moves/adjusts RuboCop exclusions in rb/.rubocop.yml (including Steepfile and Style/MixinUsage) and removes now-unneeded inline disables.
  • Adds respond_to_missing? implementations alongside method_missing usage.
  • Cleans up specs by removing a duplicated example and parameter renames for readability.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
rb/spec/integration/selenium/webdriver/spec_helper.rb Removes inline Style/MixinUsage disable and relies on config exclusion.
rb/spec/integration/selenium/webdriver/driver_spec.rb Removes duplicated RSpec example and associated inline disable.
rb/lib/selenium/webdriver/support/event_firing_bridge.rb Adds respond_to_missing? for method_missing delegation.
rb/lib/selenium/webdriver/support/color.rb Renames from_hsl parameters to descriptive names.
rb/lib/selenium/webdriver/support/block_event_listener.rb Adds respond_to_missing? for method_missing-based callback forwarding.
rb/Steepfile Removes inline Metrics/BlockLength disable (moved to RuboCop config).
rb/.rubocop.yml Updates metrics exclusions and adds Style/MixinUsage exclusion for the spec helper.

@titusfortner titusfortner merged commit 65f9304 into trunk Jan 26, 2026
60 of 61 checks passed
@titusfortner titusfortner deleted the rb_update_lint branch January 26, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-support Issue or PR related to support classes C-rb Ruby Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants