-
Notifications
You must be signed in to change notification settings - Fork 293
chore: enable #nullable iteration 2 #3167
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
Conversation
| public string? ApiName { get; set; } | ||
|
|
||
| public List<StackFrame> Frames { get; set; } | ||
| public List<StackFrame> Frames { get; set; } = null!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: !null in this context means the same as using the required keyword. We can't use the required keyword tho since its not available with our TFM we support (netstandard2.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is TFM we support ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats our TFM:
| <TargetFramework>netstandard2.0</TargetFramework> |
Its supported only in net7+
| using System.Text.Json; | ||
| using Microsoft.Playwright.Transport; | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did it go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already in line 24.
| public string? ApiName { get; set; } | ||
|
|
||
| public List<StackFrame> Frames { get; set; } | ||
| public List<StackFrame> Frames { get; set; } = null!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is TFM we support ?
| internal bool _isInternalType; | ||
|
|
||
| internal ChannelOwner(ChannelOwner parent, string guid) : this(parent, null, guid) | ||
| internal ChannelOwner(ChannelOwner parent, string guid) : this(parent, parent._connection, guid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can parent be null here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not in this scenario. It can be null when using the other overload. I'll follow-up once I touch more places aka. Connection.cs which will automatically yield this.
| keepNulls); | ||
| var message = new Dictionary<string, object> | ||
| { | ||
| ["id"] = id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this change? Previously it was type-checked, now it's just a dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove use-less wrapping type - this is similar to Python now.
| { | ||
| var createObjectInfo = message.Params.Value.ToObject<CreateObjectInfo>(DefaultJsonSerializerOptions); | ||
| CreateRemoteObject(message.Guid, createObjectInfo.Type, createObjectInfo.Guid, createObjectInfo.Initializer); | ||
| CreateRemoteObject(message.Guid, message.Params.Value.GetProperty("type").ToObject<ChannelOwnerType>(), message.Params.GetString("guid", false)!, message.Params?.GetProperty("initializer")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, the previous code was arguably more type-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally its similar and more explicit without going through the JsonSerializer infra + we don't need this extra type. I think for use-cases where we don't need to share a type this is preferred (like we do in other places as well).
| public string Name { get; set; } = null!; | ||
|
|
||
| public string MimeType { get; set; } | ||
| public string MimeType { get; set; } = null!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null! is a very indirect way to say that the field is required!
#3163