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] Improve format of generated CDP types #15129

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Jan 21, 2025

Description

Commands

Before:
image

After:
image

Domain

Before:
image
image

After:
image
image

Event

Before:
image

After:
image

Motivation and 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.

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

Event Handler

The event handler pattern was changed to use pattern matching, but the empty block after the if condition could lead to silently ignored events. Consider removing the extra block or adding error logging.

if (rawEventArgs is {{dehumanize Name}}EventArgs e)
{
    {{dehumanize Name}}?.Invoke(this, e);
}

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add thread-safe event invocation

Add null check for the event handler before invoking it to prevent potential race
conditions in multi-threaded scenarios.

third_party/dotnet/devtools/src/generator/Templates/domain.hbs [72-75]

 if (rawEventArgs is {{dehumanize Name}}EventArgs e)
 {
-    {{dehumanize Name}}?.Invoke(this, e);
+    var handler = {{dehumanize Name}};
+    handler?.Invoke(this, e);
 }
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion addresses a potential thread-safety issue by capturing the event handler in a local variable before invocation, preventing race conditions in multi-threaded scenarios where the handler could be unsubscribed between the null check and invocation.

8

@RenderMichael RenderMichael changed the title Devtools format [dotnet] Improve format of generated CDP types Jan 22, 2025
@RenderMichael RenderMichael merged commit 99df4ba into SeleniumHQ:trunk Jan 22, 2025
10 checks passed
@RenderMichael RenderMichael deleted the devtools-format branch January 22, 2025 17:52
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