-
Notifications
You must be signed in to change notification settings - Fork 184
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
Draft of typed getExtension #357
base: master
Are you sure you want to change the base?
Conversation
@@ -5,7 +5,8 @@ | |||
part of protobuf; | |||
|
|||
/// An object representing a protobuf message field. | |||
class FieldInfo<T> { | |||
/// For a repeated field S is List<T> for a non-repeated field S=T. |
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.
I see some type parameters S and U throughout the PR. Are they meant to be separate types? The comment above mentions S, but the parameter is U here. I am guessing either is meant to represent the List as mentioned
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.
Sorry - I did a rename, but forgot to rename the comment.
@@ -439,7 +439,7 @@ class _FieldSet { | |||
List<T> _$getList<T>(int index) { | |||
var value = _values[index]; | |||
if (value != null) return value as List<T>; | |||
return _getDefaultList<T>(_nonExtensionInfoByIndex(index)); | |||
return _getDefaultList<T, dynamic>(_nonExtensionInfoByIndex(index)); |
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 would this be dynamic
and not List<T>
here?
@@ -236,7 +236,7 @@ class _FieldSet { | |||
return value; | |||
} | |||
|
|||
List<T> _getDefaultList<T>(FieldInfo<T> fi) { | |||
List<T> _getDefaultList<T, U>(FieldInfo<T, U> fi) { |
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.
Is the new parameter plumbed through anywhere from here?
👀 |
Do you remember what problem this is solving @sigurdm? PR description doesn't refer to any issues. |
Currently you get back a I don't think I'll find the time to pursue this - feel free to close. |
cc: @nichite