Skip to content
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

[dotnet] Add nullability to Logs API #14875

Merged
merged 2 commits into from
Dec 8, 2024
Merged

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 8, 2024

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

Adds nullable reference type annotations to logs API.

Motivation and Context

Contributes to #14640

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

enhancement, error handling


Description

  • Enabled nullable reference types across multiple files in the Logs API.
  • Added ArgumentNullException checks for parameters that should not be null, improving error handling.
  • Converted fields to auto-properties with default values in LogEntry.
  • Improved dictionary parsing and response handling with null checks to ensure robustness.

Changes walkthrough 📝

Relevant files
Enhancement
ILogs.cs
Enable nullability and add argument null check in ILogs   

dotnet/src/webdriver/ILogs.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for logKind parameter.
  • +4/-0     
    LogEntry.cs
    Enable nullability and improve LogEntry parsing                   

    dotnet/src/webdriver/LogEntry.cs

  • Enabled nullable reference types.
  • Converted fields to auto-properties with default values.
  • Improved dictionary parsing with null checks.
  • +19/-28 
    LogLevel.cs
    Enable nullability in LogLevel                                                     

    dotnet/src/webdriver/LogLevel.cs

    • Enabled nullable reference types.
    +2/-0     
    Logs.cs
    Enable nullability and add argument null checks in Logs   

    dotnet/src/webdriver/Logs.cs

  • Enabled nullable reference types.
  • Added ArgumentNullException for driver and logKind.
  • Improved response handling with null checks.
  • +16/-10 

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 8, 2024

    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

    Potential Bug
    The ToString() method still uses old field names (timestamp, level, message) instead of the new auto-properties (Timestamp, Level, Message). This could cause incorrect string formatting.

    Error Handling
    The GetLog method should validate that commandResponse.Value is not null before attempting to cast it to object?[]. Current implementation may throw NullReferenceException if response value is null.

    Copy link
    Contributor

    qodo-merge-pro bot commented Dec 8, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect field references in string formatting to use the correct property names

    The ToString() method is using old field names (timestamp, level, message) that were
    replaced with properties. Update the method to use the new property names.

    dotnet/src/webdriver/LogEntry.cs [64]

    -return string.Format(CultureInfo.InvariantCulture, "[{0:yyyy-MM-ddTHH:mm:ssZ}] [{1}] {2}", this.timestamp, this.level, this.message);
    +return string.Format(CultureInfo.InvariantCulture, "[{0:yyyy-MM-ddTHH:mm:ssZ}] [{1}] {2}", this.Timestamp, this.Level, this.Message);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion fixes a critical bug where the ToString() method is using old field names that no longer exist after the conversion to properties, which would cause runtime errors.

    8
    General
    Ensure dictionary type matches nullable context to prevent potential runtime errors

    The dictionary parameters should be marked as nullable since it's being used with
    nullable types. Change the type to Dictionary<string, object?>.

    dotnet/src/webdriver/Logs.cs [89]

    -Dictionary<string, object> parameters = new Dictionary<string, object>();
    +Dictionary<string, object?> parameters = new Dictionary<string, object?>();
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves type safety by making the dictionary's value type nullable to match the context where it's used, preventing potential null reference issues.

    7

    💡 Need additional feedback ? start a PR chat

    Copy link
    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 @RenderMichael !

    @nvborisenko nvborisenko merged commit 623c4aa into SeleniumHQ:trunk Dec 8, 2024
    9 of 10 checks passed
    @RenderMichael RenderMichael deleted the logs branch December 9, 2024 15:14
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants