[C#] Update GetExtension to support getting typed value #9655
Conversation
| @@ -6,7 +6,7 @@ | |||
| and without the internal visibility from the test project (all of which have caused issues in the past). | |||
| --> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>net45;netstandard1.1;netstandard2.0</TargetFrameworks> | |||
| <TargetFrameworks>net462;netstandard1.1;netstandard2.0</TargetFrameworks> | |||
There was a problem hiding this comment.
Updated to net462 because VS2022 doesn't support net45.
net462 is the oldest supported netfx version in April 2022.
There was a problem hiding this comment.
Hmm... I haven't had that problem. I've been able to run tests from VS2022 without issues, as far as I'm aware. I'm okay with it though, given that .NET 4.5 (as opposed to 4.5.2) is long out of support, and even 4.5.2 isn't supported for much longer.
There was a problem hiding this comment.
It's not a problem if you install VS2022 and VS2019 side-by-side. VS2019 lets you install the bits and VS2022 can then use them.
|
Test GeneratedCodeCompilesWithOldCsharpCompiler failed on my machine. Not sure why. |
| @@ -6,7 +6,7 @@ | |||
| and without the internal visibility from the test project (all of which have caused issues in the past). | |||
| --> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>net45;netstandard1.1;netstandard2.0</TargetFrameworks> | |||
| <TargetFrameworks>net462;netstandard1.1;netstandard2.0</TargetFrameworks> | |||
There was a problem hiding this comment.
Hmm... I haven't had that problem. I've been able to run tests from VS2022 without issues, as far as I'm aware. I'm okay with it though, given that .NET 4.5 (as opposed to 4.5.2) is long out of support, and even 4.5.2 isn't supported for much longer.
| } | ||
| else | ||
| { | ||
| var storedType = value.GetType().GetTypeInfo().GenericTypeArguments[0]; |
There was a problem hiding this comment.
I think it would be worth leaving a comment here explaining why we believe all IExtensionValue values will have generic type arguments - and add a comment to IExtensionValue to advise that anyone creating a non-generic IExtensionValue implementation should change this code. Alternatively, make this code more defensive.
There was a problem hiding this comment.
Made it more defensive.
Hmm... I won't be able to look at that right now, but I'll check it tomorrow. |
|
Please could you add some more details in the PR description that's suitable for release notes? |
Done
It also fails without any changes (other than updating net451 to net462). Perhaps it is because I only have VS2022 on this machine. |
There was a problem hiding this comment.
For completeness maybe improve the method for repeated extensions as well?
- At least the error message can be improved in a similar fashion.
- Unless I'm missing something, which very well may be: Maybe even check that the actual type of the elements on the RepeatedField is assignable to TValue and return a new
RepeatedField<TValue>. This will allow users to get theRepeatedField<object>and do the parsing from bytes here as well.
|
I updated the get method for repeated values with the improved error message. Creating a new collection doesn't seem right. For example, if a new collection was returned then items added to it wouldn't be in the real collection. If someone wants to work around this issue they can use the extension method for non-repeated values with a type of |
|
Interestingly, Kokoro is failing the "GeneratedCodeCompilesWithOldCsharpCompiler" test as well - I'll have a look at that tomorrow. But this PR LGTM. |
|
Kokoro is broken in Windows. and also Guessing, I think the net45 target being on the tests projects was forcing the Google.Protobuf.dll to be build in net45, and when you removed that, it's not beeing build anymore so the test is failing? @jskeet or @JamesNK let me know if you want me to look at it. |
|
Thanks for looking at this. Test fixed by changing the path to net462. |
|
Aha, well spotted. Right, given that everything's passing, I'll squash and merge this now. |
|
Done - thanks @JamesNK! |
Fixes #9626
@jskeet