You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moving nested DTO type ChannelLocalValue.ChannelProperties to a standalone ChannelProperties type
Requires further human verification:
Verify if there are other nested types in the codebase that should be decoupled as part of this ticket
Check if the PR maintains backward compatibility or if breaking changes are properly documented
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review
Incomplete Refactoring The PR decouples ChannelProperties but leaves a TODO comment in ChannelLocalValue. This comment should be addressed or explained as part of the refactoring.
The type field is marked with a TODO comment but appears to be important for JSON serialization. Consider documenting its purpose clearly or removing the TODO comment if the field is necessary for proper serialization.
public record ChannelLocalValue(ChannelProperties Value) : LocalValue
{
- // TODO: Revise why we need it
[JsonInclude]
internal string type = "channel";
}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 3
__
Why: The suggestion correctly identifies an unclear TODO comment that could be removed for cleaner code. While valid, this is a minor improvement that doesn't affect functionality, just code clarity.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Motivation and Context
Contributes to #15407
Types of changes
Checklist
PR Type
Enhancement
Description
Decoupled
ChannelPropertiesfromChannelLocalValuein theScriptmodule.Introduced a standalone
ChannelPropertiesrecord in a new file.Simplified and improved maintainability of
ChannelLocalValue.Changes walkthrough 📝
ChannelProperties.cs
Introduced standalone `ChannelProperties` recorddotnet/src/webdriver/BiDi/Modules/Script/ChannelProperties.cs
ChannelPropertiesrecord.SerializationOptionsandOwnership.LocalValue.cs
Refactored `ChannelLocalValue` to use standalone `ChannelProperties`dotnet/src/webdriver/BiDi/Modules/Script/LocalValue.cs
ChannelPropertieswith standalone version.ChannelPropertiesdefinition.ChannelLocalValueto use the newChannelProperties.