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] Make internal console writer more flexible via taking TextWriter only #15346

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Feb 27, 2025

User description

Console is an output. We can be more extendable/abstract/generic.

Motivation and Context

Taking minimal required.

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


Description

  • Replaced ConsoleLogHandler with TextWriterHandler for flexibility.

  • Updated LogContextManager to use TextWriterHandler.

  • Modified logging tests to reflect the new handler.

  • Improved abstraction by utilizing TextWriter for log output.


Changes walkthrough 📝

Relevant files
Enhancement
LogContextManager.cs
Refactored LogContextManager to use TextWriterHandler       

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

  • Replaced ConsoleLogHandler with TextWriterHandler.
  • Updated GlobalContext initialization to use the new handler.
  • Simplified AsyncLocal initialization syntax.
  • +4/-3     
    TextWriterHandler.cs
    Introduced TextWriterHandler for flexible log output         

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

  • Renamed ConsoleLogHandler to TextWriterHandler.
  • Replaced Console.Error with a TextWriter parameter.
  • Updated log writing logic to use TextWriter.
  • +4/-4     
    Tests
    LogTest.cs
    Updated logging tests for TextWriterHandler                           

    dotnet/test/common/Internal/Logging/LogTest.cs

  • Updated tests to use TextWriterHandler instead of ConsoleLogHandler.
  • Adjusted global log reset logic to reflect handler changes.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Sorry, something went wrong.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Resource Management

    The TextWriter instance passed to TextWriterHandler is not disposed or managed by the handler. Consider implementing IDisposable or documenting that the caller is responsible for managing the TextWriter lifecycle.

    public class TextWriterHandler(TextWriter writer) : ILogHandler
    {

    Copy link
    Contributor

    qodo-merge-pro bot commented Feb 27, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check for writer

    Add null check for the TextWriter parameter to prevent NullReferenceException
    when writing logs.

    dotnet/src/webdriver/Internal/Logging/TextWriterHandler.cs [29-32]

     public class TextWriterHandler(TextWriter writer) : ILogHandler
     {
    +    private readonly TextWriter _writer = writer ?? throw new ArgumentNullException(nameof(writer));
         // performance trick to avoid expensive Enum.ToString() with fixed length
         private static readonly string[] _levels = { "TRACE", "DEBUG", " INFO", " WARN", "ERROR" };
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: Adding a null check for the TextWriter parameter is crucial for preventing potential NullReferenceException during runtime, which could cause application crashes. This is a important defensive programming practice for a core logging component.

    Medium
    • Update

    Sorry, something went wrong.

    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

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

    Having the TextWriterHandler is a good idea, however the existing ConsoleLogHandler is used by users. We would need to keep a type that just points to the text writer handler:

    public class ConsoleLogHandler : TestWriterHandler(Console.Error);

    Additionally, TestWriterHandler(Console.Error) is not the same as ConsoleLogHandler if a user uses the Console.SetError method. The current code would point to whatever the new Console.Error pointed to, but this PR would still point to the old sink.

    @nvborisenko
    Copy link
    Member Author

    nvborisenko commented Feb 27, 2025

    Additionally, TestWriterHandler(Console.Error) is not the same as ConsoleLogHandler if a user uses the Console.SetError method. The current code would point to whatever the new Console.Error pointed to, but this PR would still point to the old sink.

    True, and it is still good. As a low-level library we want to give user a power. Since this is "internal/advanced" thing, I am OK with this behavior.

    however the existing ConsoleLogHandler is used by users

    OK, let's keep it and proceed via standard deprecation policy. Thank you, it is really better than just break.

    @nvborisenko nvborisenko merged commit 5ca1cc0 into SeleniumHQ:trunk Feb 27, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-log-console-rework branch February 27, 2025 22:13
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    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.

    None yet

    2 participants