525 add helpers to allow configuration of services without the need of a host #579
Conversation
…eter---cs-typeformat
…at' of https://github.com/FirelyTeam/firely-cql-sdk into 573-remove-obsolete-commandline-parameter---cs-typeformat
Demo/CqlSdkUseCases/Program.cs
Outdated
| ServiceCollection services = new ServiceCollection(); | ||
| services.AddLogging(lb => lb.ClearProviders()); | ||
| services.AddCqlCodeGenerationServices(); | ||
| services.AddScoped<ElmToCSharpFactory>(); |
There was a problem hiding this comment.
Why is this scoped? Scoped services make usage much harder and worsen performance. Average programmers are completely unaware of what scopes are, don't understand how IDisposable works or why you would need to wrap a scope in a using block in some cases, etc. Even on line 26 you create a scope that persists for the remainder of the program and if you tried copying line 26 later on to process another library set, even I don't know what would happen and I've been using dependency injection for more than 15 years. I would strongly suggest not having scoped services. I tried using some scoped services in CqlToElm and regretted it almost immediately. I had tons of state errors as a result of services that I thought would be regenerated not being regenerated. When you combine singleton and scoped services, you get really weird behavior that is hard to debug. I think we should remove the scoping and change whatever usage that results, e.g., move any class state into parameters to the functions that need that state.
Demo/CqlSdkUseCases/Program.cs
Outdated
| dllDir.Create(); | ||
| ElmToCSharp elmToCSharp = elmToCSharpFactory.ForLibrarySet(librarySet); | ||
| CSharpToBinary cSharpToBinary = cSharpToBinaryFactory.ForLibrarySet(librarySet); | ||
| await foreach (var (library, csharpCodeStream) in elmToCSharp.GenerateCSharp(CancellationToken.None)) |
There was a problem hiding this comment.
This usage is too hard. The public incarnation of GenerateCSharp should return source code strings, not streams, and not tuples - it should return an IDictionary which are much easier to use.
Alternatively, we could take this loop and make it a public function on ElmToCSharp like WriteCodeTo(DirectoryInfo) so we can write them an efficient parallel loop that can operate on the streams.
Even if we take this approach, we need to give people the opportunity to access the generated the CSharp as an IDictionary<VersionedIdentifier,string>.
Demo/CqlSdkUseCases/Program.cs
Outdated
| } | ||
|
|
||
|
|
||
| Console.WriteLine("Goodbye"); |
Demo/CqlSdkUseCases/Program.cs
Outdated
| DirectoryInfo elmDir = new DirectoryInfo("C:\\Dev\\firely-cql-sdk\\LibrarySets\\Demo\\Elm"); | ||
| DirectoryInfo csharpDir = new DirectoryInfo("CSharp"); | ||
| DirectoryInfo dllDir = new DirectoryInfo("Dll"); | ||
| LibrarySet librarySet = new LibrarySet("Example Library"); |
There was a problem hiding this comment.
We should write a few more comments here, like
// If you want to compile a library, you need all of its dependencies in the same LibrarySet
As a side note, I almost never know what to name a LibrarySet and use "" most of the time. Suggest making the name parameter of LibrarySet optional.
Demo/CqlSdkUseCases/Program.cs
Outdated
| _librarySet = librarySet; | ||
| } | ||
|
|
||
| public async IAsyncEnumerable<(Library library, Stream csharpCodeStream)> GenerateCSharp(CancellationToken ct) |
There was a problem hiding this comment.
Async methods should be suffixed with the word Async like the C# BCL does. See this page.
Demo/CqlSdkUseCases/Program.cs
Outdated
| } | ||
| } | ||
|
|
||
| await processTask; |
There was a problem hiding this comment.
I've never seen an async method implemented like this before. I can pinpoint a couple things. For example, this function should not be async and you shouldn't await processTask on line 143 and just return processTask instead. This lets the caller do the same kind of shenanigans you're doing with tasks here, e.g., they could combine the returned Task from this method with other tasks and then use Task.WaitAll() on them. I think this is just wrong, but I am having a hard time articulating exactly why, except that I've never seen code that looks anything like this, and that's a signal to me that something is wrong.
I think we need to take this back to the drawing board. The CSharpSourceCodeWriterCallbacks concept is overengineered and I think this method here proves that point. Look at how complicated this code is.
We should discuss how to rescue the assembly generator. The code is inherently complicated and requires these steps, but there's a default set of steps that everyone will use 99% of the time. We should perhaps consider an event pattern here.
Also, nobody ever uses CancellationToken, and certainly not as a mandatory parameter. 99.9% of users will not cancel this task. If we must (e.g., I could foresee Firely Server wanting to be able to provide a token in case this is taking too long), then make it optional.
There was a problem hiding this comment.
This PR was in draft and not nearly ready for a technical review yet. It was more to get a conversation going on the public API, not the internals.
|
I've started with that a bit already in this PR, but the focus is first on getting the public API, and then fix the internals as it makes sense
|
|
BTW, I will still design a whole public facade around everything so that the user won't have to care about services and scopes. If we have a clean simple public facade, we can do a lot to clean up the internals more efficiently. Regarding scopes, initially this was necessary to have for each libraryset (like a scope is maintained for each http request in asp.net). This is no longer needed, so that needs to be cleaned up too Also, I don't this IAsyncEnumerable is the right strategy for processing elm into resource. I have some ideas around this |
Conceptual.
Thinking how we can open up the packaging tooling public. The idea is to wrap up the messy internals into something that hides this (and the IServiceProvider from the user)
Questions for the public facade