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] Annotate nullable reference types in internal logging #14819

Merged
merged 3 commits into from
Dec 1, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 27, 2024

User description

…al.Logging`

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

Annotate nullable reference types in OpenQA.Selenium.Internal.Logging

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, bug_fix


Description

  • Enabled nullable reference types across multiple files in the OpenQA.Selenium.Internal.Logging namespace to improve code safety and clarity.
  • Added exception handling for null or empty parameters in constructors and methods to prevent runtime errors.
  • Refactored properties to use expression-bodied members for cleaner and more concise code.
  • Sealed classes LogHandlerList and Logger to prevent further inheritance, ensuring better encapsulation.

Changes walkthrough 📝

Relevant files
Enhancement
13 files
ConsoleLogHandler.cs
Enable nullable reference types in ConsoleLogHandler         

dotnet/src/webdriver/Internal/Logging/ConsoleLogHandler.cs

  • Enabled nullable reference types.
+2/-0     
FileLogHandler.cs
Enhance nullability and exception handling in FileLogHandler

dotnet/src/webdriver/Internal/Logging/FileLogHandler.cs

  • Enabled nullable reference types.
  • Added exception handling for null or empty file paths.
  • Improved nullability handling in Dispose method.
  • +12/-3   
    ILogContext.cs
    Enable nullable reference types in ILogContext                     

    dotnet/src/webdriver/Internal/Logging/ILogContext.cs

    • Enabled nullable reference types.
    +2/-0     
    ILogHandler.cs
    Enable nullable reference types in ILogHandler                     

    dotnet/src/webdriver/Internal/Logging/ILogHandler.cs

    • Enabled nullable reference types.
    +2/-0     
    ILogHandlerList.cs
    Enable nullable reference types in ILogHandlerList             

    dotnet/src/webdriver/Internal/Logging/ILogHandlerList.cs

    • Enabled nullable reference types.
    +2/-0     
    ILogger.cs
    Enable nullable reference types in ILogger                             

    dotnet/src/webdriver/Internal/Logging/ILogger.cs

    • Enabled nullable reference types.
    +2/-0     
    Log.cs
    Enable nullable reference types and refactor properties in Log

    dotnet/src/webdriver/Internal/Logging/Log.cs

  • Enabled nullable reference types.
  • Used expression-bodied members for properties.
  • +6/-8     
    LogContext.cs
    Enhance nullability handling in LogContext                             

    dotnet/src/webdriver/Internal/Logging/LogContext.cs

  • Enabled nullable reference types.
  • Improved nullability handling in methods.
  • +10/-17 
    LogContextManager.cs
    Enable nullable reference types and refactor properties in
    LogContextManager

    dotnet/src/webdriver/Internal/Logging/LogContextManager.cs

  • Enabled nullable reference types.
  • Used expression-bodied members for properties.
  • +9/-23   
    LogEvent.cs
    Enhance nullability and exception handling in LogEvent     

    dotnet/src/webdriver/Internal/Logging/LogEvent.cs

  • Enabled nullable reference types.
  • Added exception handling for null parameters.
  • +5/-2     
    LogEventLevel.cs
    Enable nullable reference types in LogEventLevel                 

    dotnet/src/webdriver/Internal/Logging/LogEventLevel.cs

    • Enabled nullable reference types.
    +2/-0     
    LogHandlerList.cs
    Enable nullable reference types and seal LogHandlerList   

    dotnet/src/webdriver/Internal/Logging/LogHandlerList.cs

  • Enabled nullable reference types.
  • Sealed the LogHandlerList class.
  • +3/-1     
    Logger.cs
    Enable nullable reference types and seal Logger                   

    dotnet/src/webdriver/Internal/Logging/Logger.cs

    • Enabled nullable reference types.
    • Sealed the Logger class.
    +3/-1     

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

    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

    Null Reference
    The Handle method could throw ObjectDisposedException if _streamWriter is null, but the null check may not be sufficient if disposal happens concurrently. Consider additional synchronization.

    Thread Safety
    The _loggers dictionary initialization is not thread-safe. Multiple threads could create multiple dictionaries concurrently. Consider using a lock or Lazy.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Nov 27, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    ✅ Simplify null check using null-coalescing operator for better readability and maintainability
    Suggestion Impact:The null check for _streamWriter was removed, simplifying the code, although the null-coalescing operator was not used. The impact aligns with the suggestion's intent to simplify the null check.

    code diff:

    -            if (_streamWriter is not { } writer)
    -            {
    -                throw new ObjectDisposedException(nameof(FileLogHandler));
    -            }
    -
                 lock (_lockObj)
                 {
    -                writer.WriteLine($"{logEvent.Timestamp:yyyy-MM-dd HH:mm:ss.fff} {_levels[(int)logEvent.Level]} {logEvent.IssuedBy.Name}: {logEvent.Message}");
    +                _streamWriter.WriteLine($"{logEvent.Timestamp:yyyy-MM-dd HH:mm:ss.fff} {_levels[(int)logEvent.Level]} {logEvent.IssuedBy.Name}: {logEvent.Message}");

    The null-coalescing operator (??) should be used instead of the pattern matching
    expression to check for null StreamWriter, as it's more idiomatic in C# and clearer
    in intent.

    dotnet/src/webdriver/Internal/Logging/FileLogHandler.cs [83-91]

    -if (_streamWriter is not { } writer)
    -{
    -    throw new ObjectDisposedException(nameof(FileLogHandler));
    -}
    +var writer = _streamWriter ?? throw new ObjectDisposedException(nameof(FileLogHandler));
     
     lock (_lockObj)
     {
         writer.WriteLine($"{logEvent.Timestamp:yyyy-MM-dd HH:mm:ss.fff} {_levels[(int)logEvent.Level]} {logEvent.IssuedBy.Name}: {logEvent.Message}");
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves code readability by using a more concise and idiomatic C# pattern. While valid, the impact is moderate as it's primarily a stylistic change.

    5

    💡 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.

    Thank you @RenderMichael for contribution!

    @nvborisenko
    Copy link
    Member

    /automerge please

    @nvborisenko nvborisenko merged commit 4f07e4a into SeleniumHQ:trunk Dec 1, 2024
    10 checks passed
    @RenderMichael RenderMichael deleted the logging branch December 1, 2024 22:04
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 2, 2024
    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