Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Mar 8, 2025

User description

Description

Remove #nullable enable from CDP-generated types

Motivation and Context

Missed cleanup from the nullable adventure.

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

  • Removed #nullable enable directives from multiple CDP-generated templates.

  • Simplified property getter syntax in DevToolsSessionDomains.hbs.

  • Replaced dictionary key lookup with TryGetValue in domain.hbs.

  • Ensured consistent formatting and cleanup across templates.


Changes walkthrough 📝

Relevant files
Enhancement
DevToolsSessionDomains.hbs
Simplified property getter syntax                                               

third_party/dotnet/devtools/src/generator/Templates/DevToolsSessionDomains.hbs

  • Simplified property getter syntax using expression-bodied members.
+1/-4     
command.hbs
Removed `#nullable enable` directive                                         

third_party/dotnet/devtools/src/generator/Templates/command.hbs

  • Removed #nullable enable directive.
+0/-2     
domain.hbs
Removed `#nullable enable` and optimized key lookup           

third_party/dotnet/devtools/src/generator/Templates/domain.hbs

  • Removed #nullable enable directive.
  • Replaced dictionary key lookup with TryGetValue for better
    performance.
  • +1/-4     
    event.hbs
    Removed `#nullable enable` directive                                         

    third_party/dotnet/devtools/src/generator/Templates/event.hbs

    • Removed #nullable enable directive.
    +0/-2     
    type-enum.hbs
    Removed `#nullable enable` directive                                         

    third_party/dotnet/devtools/src/generator/Templates/type-enum.hbs

    • Removed #nullable enable directive.
    +0/-2     
    type-hash.hbs
    Removed `#nullable enable` directive                                         

    third_party/dotnet/devtools/src/generator/Templates/type-hash.hbs

    • Removed #nullable enable directive.
    +0/-2     
    type-object.hbs
    Removed `#nullable enable` directive                                         

    third_party/dotnet/devtools/src/generator/Templates/type-object.hbs

    • Removed #nullable enable directive.
    +0/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Null Reference

    The TryGetValue method is used but the eventData variable is referenced directly afterward without checking if it exists. This could lead to a null reference exception if the event name is not found in the dictionary.

    if (m_eventMap.TryGetValue(e.EventName, out var eventData))
    {
        var eventArgs = e.EventData.Deserialize(eventData.EventArgsType, global::OpenQA.Selenium.DevTools.Json.DevToolsJsonOptions.Default);
        eventData.EventInvoker(eventArgs);
    }

    @qodo-code-review
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add null check

    The code is missing a check for null eventData after the TryGetValue call. While
    the method correctly uses TryGetValue to safely retrieve the value, it doesn't
    check if the retrieved value is valid before using it.

    third_party/dotnet/devtools/src/generator/Templates/domain.hbs [59-68]

     private void OnDevToolsEventReceived(object? sender, DevToolsEventReceivedEventArgs e)
     {
         if (e.DomainName == m_domainName)
         {
    -        if (m_eventMap.TryGetValue(e.EventName, out var eventData))
    +        if (m_eventMap.TryGetValue(e.EventName, out var eventData) && eventData != null)
             {
                 var eventArgs = e.EventData.Deserialize(eventData.EventArgsType, global::OpenQA.Selenium.DevTools.Json.DevToolsJsonOptions.Default);
                 eventData.EventInvoker(eventArgs);
             }
         }
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion adds a null check for the eventData variable after TryGetValue, which is a good defensive programming practice. While TryGetValue already ensures we only proceed if the key exists, the additional null check provides extra protection against potential null reference exceptions.

    Low
    • More

    @RenderMichael
    Copy link
    Contributor Author

    This is triggering a lot of warnings. This change doesn't add much value, and if it's not valid cleanup then I will close this PR.

    @RenderMichael RenderMichael deleted the nullable-cdp branch March 8, 2025 05:00
    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.

    1 participant