-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Follow up on IParsable changes and update tests #47102
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
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.
Looks good. Minor comment around whether we should use CodeWriter for the injected methods, but not a big issue given the way they are used and where they are injected.
@@ -19,7 +19,97 @@ internal static class RequestDelegateGeneratorSources | |||
|
|||
public static string GeneratedCodeAttribute => $@"[System.CodeDom.Compiler.GeneratedCodeAttribute(""{typeof(RequestDelegateGeneratorSources).Assembly.FullName}"", ""{typeof(RequestDelegateGeneratorSources).Assembly.GetName().Version}"")]"; | |||
|
|||
public static string GetGeneratedRouteBuilderExtensionsSource(string genericThunks, string thunks, string endpoints) => $$""" | |||
public static string TryResolveBodyAsyncMethod => """ | |||
private static async ValueTask<(bool, T?)> TryResolveBodyAsync<T>(HttpContext httpContext, bool allowEmpty) |
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.
Should these strings be cut over to CodeWriter implementations. Given where these get emitted, we can pretty easily predict the indentation requirements but really just asking for the sake of consistency since they are multi-line blocks.
src/Http/Http.Extensions/test/Microsoft.AspNetCore.Http.Extensions.Tests.csproj
Show resolved
Hide resolved
...test/RequestDelegateGenerator/Baselines/MapAction_SingleEnumParam_StringReturn.generated.txt
Show resolved
Hide resolved
<AssemblyAttribute | ||
Include="System.Reflection.AssemblyMetadataAttribute"> | ||
<_Parameter1>RequestDelegateGeneratorTestBaselines</_Parameter1> | ||
<_Parameter2>$([System.IO.Path]::Combine($(MSBuildProjectDirectory), "RequestDelegateGenerator", "Baselines"))</_Parameter2> |
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.
Does this work on things like Helix?
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.
Yep, we use this pattern in our E2E tests. There's a little bit more work that I have to do to though get things wired up correctly in the MSBuild, apparently.
|
||
public static string TryParseExplicitMethod => """ | ||
private static bool TryParseExplicit<T>(string? s, IFormatProvider? provider, [MaybeNullWhen(returnValue: false)] out T result) where T: IParsable<T> | ||
=> T.TryParse(s, provider, out result); |
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.
😲 does RDG now support something RDF doesn'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.
Yep, this was part of the original TryParse
support in RDG. IMO, we shouldn't not do something in the RDG just because the RDF doesn't support it yet...
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.
We should file an issue to support it on both.
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.
IParsable
-related feedback from [main] Update dependencies from dotnet/runtime #47021