[rb] handled nil condition for Cookie Method in ruby

[pallavigitwork]

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

implemented handling nil/empty for cookie methods - reference pr- #15044

Motivation and Context

it wasn't implemented in ruby

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.

implemented handling nil/empty for cookie methods - reference pr- #15044

---

PR Type

Bug fix, Enhancement

---

Description

• Added validation for nil or empty cookie names in delete_cookie and cookie methods.

• Raised Error::ArgumentError for invalid cookie names.

• Improved error handling for cookie-related methods in Ruby WebDriver.

---

Changes walkthrough 📝

	Relevant files
Bug fix	bridge.rb Validate cookie name in Ruby WebDriver methods                      rb/lib/selenium/webdriver/remote/bridge.rb Added checks for nil or empty cookie names in delete_cookie and cookie methods. Raised Error::ArgumentError with a descriptive message for invalid names. +6/-0

---

Need help?

Type /help how to ... in the comments thread for any question about Qodo Merge usage.

Check out the documentation for more information.

[qodo-merge-pro]

PR Reviewer Guide 🔍

(Review updated until commit 10cc4d3)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 15044 - Partially compliant Compliant requirements: Handle null/empty cookie name validation Raise appropriate error for invalid cookie names Non-compliant requirements: Improve cookie retrieval efficiency by using dedicated endpoint instead of fetching all cookies

⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Style Extra parentheses around condition in if statement are unnecessary in Ruby and should be removed for better readability if ( name.nil? || name.empty?) then

[qodo-merge-pro]

PR Code Suggestions ✨

Latest suggestions up to 10cc4d3
Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	✅ Add type safety by ensuring string conversion before checking for emptiness Suggestion Impact: The commit modified the same code section but implemented a different logic for checking empty cookie names using 'unless name || !name.empty?' code diff: - if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null or empty' - end + raise ArgumentError, 'Cookie name cannot be null or empty' unless name || !name.empty? Consider using String#to_s before checking empty? to handle non-string inputs gracefully, preventing potential NoMethodError. rb/lib/selenium/webdriver/remote/bridge.rb [388-390] -if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null or empty' -end +raise Error::ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.to_s.empty? [Suggestion has been applied] Suggestion importance[1-10]: 8 Why: This is a valuable defensive programming suggestion that prevents potential runtime errors by safely handling non-string inputs, making the code more robust and preventing potential NoMethodError exceptions.	8
General	✅ Simplify conditional statement by following idiomatic Ruby style and using guard clause pattern Suggestion Impact: The commit directly implemented the suggested changes by converting the if-then blocks to guard clauses and removing unnecessary parentheses in both delete_cookie and cookie methods code diff: - if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null or empty' - end + raise Error::ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.empty? execute :delete_cookie, name: name end def cookie(name) - if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null or empty' - end + raise Error::ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.empty? Remove unnecessary parentheses around the condition and redundant 'then' keyword to follow Ruby style conventions. Also, consider using a guard clause pattern for better readability. rb/lib/selenium/webdriver/remote/bridge.rb [388-390] -if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null or empty' -end +raise Error::ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.empty? Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion improves code readability and follows Ruby conventions by removing unnecessary parentheses and 'then' keyword, using a more idiomatic guard clause pattern. While valid, this is primarily a style improvement.	5

---

Previous suggestions

✅ Suggestions up to commit 3966bfe

Category	Suggestion	Score
General	✅ Improve error message accuracy to match the actual validation being performed Suggestion Impact: The commit implemented the suggested error message change in both delete_cookie and cookie methods, making the error messages more accurate by explicitly mentioning both null and empty conditions code diff: if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null' + raise Error::ArgumentError, 'Cookie name cannot be null or empty' end execute :delete_cookie, name: name end def cookie(name) if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null' + raise Error::ArgumentError, 'Cookie name cannot be null or empty' end Update error message to accurately reflect both nil and empty string conditions being checked. rb/lib/selenium/webdriver/remote/bridge.rb [389] -raise Error::ArgumentError, 'Cookie name cannot be null' +raise Error::ArgumentError, 'Cookie name cannot be null or empty' Suggestion importance[1-10]: 7 Why: The suggestion improves the error message accuracy by explicitly mentioning both nil and empty conditions, which helps developers better understand the validation failure reason.	7
	✅ Improve code readability by following Ruby style conventions for conditional statements Suggestion Impact: The commit refactored the conditional statements by removing parentheses and 'then' keyword, and also improved the error message and used a more idiomatic Ruby one-line conditional code diff: - if ( name.nil? || name.empty?) then - raise Error::ArgumentError, 'Cookie name cannot be null' - end + raise Error::ArgumentError, 'Cookie name cannot be null or empty' if name.nil? || name.empty? Remove unnecessary parentheses around the condition and remove redundant 'then' keyword to follow Ruby style conventions. rb/lib/selenium/webdriver/remote/bridge.rb [388-390] -if ( name.nil? || name.empty?) then +if name.nil? || name.empty? raise Error::ArgumentError, 'Cookie name cannot be null' end Suggestion importance[1-10]: 5 Why: The suggestion correctly identifies Ruby style conventions by removing unnecessary parentheses and 'then' keyword, which improves code readability and maintainability.	5

[aguspe]

Thank you so much for the help! this is already implemented, so I will close the PR, have a great day!

[nvborisenko]

@aguspe This adds validation of cookie name to be aligned with other bindings, I think the PR is still actual.

[aguspe]

@nvborisenko I was talking with Pallavi, and It feels strange for me to add this type of validation in Ruby with a mandatory parameter.

But if the goal is to be aligned between all the bindings and return a consistent error across bindings and browsers then I can re-open this and help Pallavi with the testing

We can update the guard clause instead to make it more Ruby-like

[aguspe]

After discussing with @nvborisenko I'm re-opening this PR and adding a review comment

[pallavigitwork]

is this PR accepted @aguspe ? Please let me know? thanks

[aguspe]

@pallavigitwork this test is failing due to expected ArgumentError but nothing was raised you need to pass nil or an empty hash to make this test work

[pallavigitwork]

ok thank you @aguspe . I will try to fix this by this thursday. i will work on the feedback you gave. thank you for your time.

[pallavigitwork]

made changes. requesting review, @aguspe

[aguspe]

There is an extra space here that is making the formatted field could you run:

./scripts/format.sh

[pallavigitwork]

removed.

[pallavigitwork]

this runs.
but rubocop throws error

Error: unrecognized cop or department Lint/UselessConstantScoping found in rb/.rubocop.yml
Did you mean Lint/ConstantResolution?

do i change something in the yml file?
this- Lint/UselessConstantScoping:
Enabled: false
installed gem rubocop again. i get this message - 333 files inspected, 343 offenses detected, 244 offenses autocorrectable

[pallavigitwork]

unable to make this work. closing this one with comment as unresolved.
will be opening a new PR to try again.
this PR is requested to be kept as it is. i need some information from it for a talk. so let this be as it is. thanks