Skip to content

Renaming variable and converting it to nullable bool?#16138

Merged
diemol merged 4 commits intotrunkfrom
dotnet_rename_log_truncate_nullable
Aug 7, 2025
Merged

Renaming variable and converting it to nullable bool?#16138
diemol merged 4 commits intotrunkfrom
dotnet_rename_log_truncate_nullable

Conversation

@diemol
Copy link
Copy Markdown
Member

@diemol diemol commented Aug 6, 2025 •
edited
Loading

🔗 Related Issues

Addresses #16131 (comment)

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 6, 2025
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Aug 6, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

• Fix JavaScript execution in link href on click() for Firefox driver
• Ensure compatibility with Firefox 42.0 and Selenium 2.48.x versions
• Restore functionality that worked in version 2.47.1

5678 - Not compliant

Non-compliant requirements:

• Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4
• Resolve connection refused errors for subsequent ChromeDriver instances
• Ensure proper ChromeDriver instantiation beyond the first instance

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

Logic Error

The property name LogTruncate suggests enabling truncation when true, but the implementation adds --log-no-truncate flag when the value is true, which disables truncation. This creates confusing semantics where LogTruncate=true actually means disable truncation.

 
/// Gets or sets a value indicating whether to disable truncation of long log lines in GeckoDriver.
/// </summary>
/// <remarks>
/// A <see langword="null"/> value indicates no log truncation setting to specify.
/// Set to <see langword="true"/> to disable truncation, or <see langword="false"/> to enable truncation.
/// </remarks>
public bool? LogTruncate { get; set; }

@diemol diemol mentioned this pull request Aug 6, 2025
Merged
@diemol diemol added this to the Selenium 4.35 milestone Aug 6, 2025
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Aug 6, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix property name semantic mismatch

The property name LogTruncate contradicts its documentation and usage. The
documentation states "true to disable truncation" but the name suggests enabling
truncation. Consider renaming to LogNoTruncate or updating the logic to match
the name.

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [107-113]

-/// Gets or sets a value indicating whether to disable truncation of long log lines in GeckoDriver.
+/// Gets or sets a value indicating whether to truncate long log lines in GeckoDriver.
 /// </summary>
 /// <remarks>
 /// A <see langword="null"/> value indicates no log truncation setting to specify.
-/// Set to <see langword="true"/> to disable truncation, or <see langword="false"/> to enable truncation.
+/// Set to <see langword="true"/> to enable truncation, or <see langword="false"/> to disable truncation.
 /// </remarks>
 public bool? LogTruncate { get; set; }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant semantic mismatch where the property name LogTruncate contradicts its documentation, making the API confusing and error-prone for users.

Medium
Possible issue
Fix inverted boolean logic

The logic is inverted - when LogTruncate.Value is true, it appends
--log-no-truncate which disables truncation. This contradicts the property name
and creates confusion about the intended behavior.

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [206-209]

-if (this.LogTruncate.HasValue && this.LogTruncate.Value)
+if (this.LogTruncate.HasValue && !this.LogTruncate.Value)
 {
     argsBuilder.Append(" --log-no-truncate");
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the implementation logic is inverted with respect to the property name LogTruncate, which is a significant clarity and maintainability issue.

Medium
  • Update

nvborisenko
nvborisenko reviewed Aug 6, 2025
View reviewed changes
nvborisenko
nvborisenko approved these changes Aug 6, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm with comment.

@diemol diemol merged commit 68ebd5f into trunk Aug 7, 2025
10 checks passed
@diemol diemol deleted the dotnet_rename_log_truncate_nullable branch August 7, 2025 09:10
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

C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Milestone

Selenium 4.35

Development

Successfully merging this pull request may close these issues.

3 participants

@diemol @nvborisenko @selenium-ci