Changes Needed to Generate Sync RequestAuthenticator

[AnaCoda]

US154650: Async to Sync Generator: Generate RequestAuthenticator
Added [GenerateSync] as needed, partial classes, and small changes (made Task IRequestAuthenticator.AuthenticateAsync() asynchronous)

[j3parker]

I'd be fine with just

=> Task.Run( () => m_httpClient.getAsync( requestUri, completionOption, cancellationToken ) ).Result;

but no big deal

[j3parker]

Very cool.

Could we try [GenerateSync] on one of the tests for fun? If it works to generate a sync test that would be pretty cool, heh. If it doesn't that's fine for now but I'd be curious about why.

[omsmith]

Didn't our strategy pivot to sticking with HttpClient and generating the Task.Run wrapper?

[AnaCoda]

Correct me if I'm wrong @j3parker but I think this is a form of future-proofing so that we can implement true "sync" versions of HttpClient methods in the future. We do have the ability to wrap them in Task.Run but saved that for one-offs and not the HttpClient class

[j3parker]

I think the interface we're going to need to wrap HTTP is going to look a lot different than what we've got here (like methods like T GetAsJson<T>( string url ); etc., high level stuff).

I'm OK if we can drop D2LHttpClient for now and rely on the generator, or if that's a hassle I don't care if we have this (just it'll change drastically in a bit, that's all.)

[AnaCoda]

It is a bit of a hassle, because given the common names "Get" and "Send", I would have to check the types of whatever is calling them. Anyway, it would have to be a new package so if things are going to change drastically anyway I feel that we shouldn't make additional changes to the generator only to revert them later

[omsmith]

I guess I'm unsure why we added the ReadAsStringAsync wrapping into the generator, when it was directly related to this HttpClient usage - and I thought we were ultimately adding it to avoid having to wrap HttpClient (at the moment, at least).

Whatever works if we're just working incrementally though. I'd rather us not cut a release of D2L.Security.OAuth2 as-is though.

[AnaCoda]

I see, it's because ReadAsStringAsync is part of HttpContent not HttpClient which complicates things

[AnaCoda]

Very cool.

Could we try [GenerateSync] on one of the tests for fun? If it works to generate a sync test that would be pretty cool, heh. If it doesn't that's fine for now but I'd be curious about why.

Any tests for these changes will be in #316 (force pushed to remove them from this branch)

[omsmith]

There's a number of cases of new code not matching our existing style. Could you get that fixed up?

• opening braces shouldn't have their own line (class Foo : IFoo {)
• method declarations and invocations should have spaces within the parens (void Foo( string bar ) {, Foo( "Bar" );)

[j3parker]

I think we should ultimately wrap HttpClient in something that implements the sync workflows with HttpWebRequest. The interface for that wrapper would be pretty "simple", e.g. it wouldn't return HttpResponseMessage (that object is too complex) and tailored to our (limited needs).

If we do that we maybe don't need the Task.Run wrapping support in the generator, which would be ideal.

I don't think we need to hold up this PR on any of that though, and I'm keen to see some e2e stuff in the LMS.