Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jan 30, 2025

User description

Proper source generated json context

Motivation and Context

Road for devtools json context

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

Bug fix, Enhancement


Description

  • Added snake_case type names to avoid name collisions.

  • Updated JSON serialization attributes with snake_case type names.

  • Removed unused NuGet configuration file.

  • Enhanced code generation for commands, responses, and events.


Changes walkthrough 📝

Relevant files
Enhancement
CommandInfo.cs
Add snake_case properties to CommandInfo class                     

third_party/dotnet/devtools/src/generator/CodeGen/CommandInfo.cs

  • Introduced FullSnakeTypeName property for snake_case type names.
  • Added FullSnakeResponseTypeName property for snake_case response type
    names.
  • +4/-0     
    EventInfo.cs
    Add snake_case property to EventInfo class                             

    third_party/dotnet/devtools/src/generator/CodeGen/EventInfo.cs

    • Introduced FullSnakeTypeName property for snake_case type names.
    +2/-0     
    DevToolsSessionDomains.hbs
    Update JSON serialization attributes in templates               

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

  • Updated JSON serialization attributes to use snake_case type names.
  • Enhanced template for commands and events serialization.
  • +3/-3     
    Configuration changes
    NuGet.Config
    Remove unused NuGet configuration file                                     

    third_party/dotnet/devtools/NuGet.Config

    • Removed unused NuGet configuration file.
    +0/-6     

    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-merge-pro
    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

    Type Consistency

    Verify that the snake_case type names generated by the Replace operation maintain uniqueness and don't cause any new name collisions

        [global::System.Text.Json.Serialization.JsonSerializable(typeof({{FullTypeName}}), TypeInfoPropertyName = "{{FullSnakeTypeName}}")]
        [global::System.Text.Json.Serialization.JsonSerializable(typeof({{FullResponseTypeName}}), TypeInfoPropertyName = "{{FullSnakeResponseTypeName}}")]
    {{/each}}
    {{#each events}}
        [global::System.Text.Json.Serialization.JsonSerializable(typeof({{FullTypeName}}), TypeInfoPropertyName = "{{FullSnakeTypeName}}")]

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent null reference exceptions

    Add null check before calling Replace to prevent potential NullReferenceException
    when fullTypeName or fullResponseTypeName are null

    third_party/dotnet/devtools/src/generator/CodeGen/CommandInfo.cs [12-16]

    -public string FullSnakeTypeName { get; } = fullTypeName.Replace(".", "_");
    -public string FullSnakeResponseTypeName { get; } = fullResponseTypeName.Replace(".", "_");
    +public string FullSnakeTypeName { get; } = fullTypeName?.Replace(".", "_") ?? string.Empty;
    +public string FullSnakeResponseTypeName { get; } = fullResponseTypeName?.Replace(".", "_") ?? string.Empty;
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential runtime crash by adding null checks for fullTypeName and fullResponseTypeName parameters. This is particularly important as these values are used in constructor initialization and could lead to NullReferenceException if null values are passed.

    8

    @nvborisenko
    Copy link
    Member Author

    It fixes warnings like:

    There are multiple types named 'RequestPatternArray'. Source was generated for the first one detected. Use 'JsonSerializableAttribute.TypeInfoPropertyName' to resolve this collision. (https://learn.microsoft.com/dotnet/fundamentals/syslib-diagnostics/syslib1031)
    

    We still need add types from domains.
    For example: https://chromedevtools.github.io/devtools-protocol/tot/Fetch/
    image

    @RenderMichael
    Copy link
    Contributor

    The types should come in as transitive from wherever they are used. For example, Fetch.AuthChallenge is used by the AuthRequiredEventArgs type
    image

    @nvborisenko
    Copy link
    Member Author

    And this is an issue. We have the same name for Fetch.RequestPattern and Network.RequestPattern - name collision. We should help generator to name it uniquely.

    @RenderMichael
    Copy link
    Contributor

    Do you plan to do this in this PR, or a following one?

    @nvborisenko nvborisenko merged commit c3a70ce into SeleniumHQ:dotnet-devtools-aot Jan 30, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the dotnet-fix-collisions branch January 30, 2025 15:10
    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