.NET 10, HostApplicationBuilder in samples, ReactiveUI -> use RxAppBuilder, ...#55
.NET 10, HostApplicationBuilder in samples, ReactiveUI -> use RxAppBuilder, ...#55gszdev wants to merge 10 commits intodapplo:masterfrom
Conversation
gszdev
commented
Mar 27, 2026
gszdev
commented
- added support for .NET 10
- added support for HostApplicationBuilder to samples
- updated depencencies (nuget packages)
- ReactiveUI -> use RxAppBuilder
- added slnx files
- added support for HostApplicationBuilder to samples - updated depencencies (nuget packages) - added slnx files
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the solution to .NET 10.0, updates various dependencies to their latest versions, and introduces support for HostApplicationBuilder across several sample projects. Additionally, it migrates the solution to the .slnx format and adds a shared utility for environment configuration. Review feedback highlights a potential InvalidOperationException when parsing host settings and identifies several instances of dead code, including unused constants and extension methods, that should be removed for better maintainability.
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request updates the project to support .NET 10, upgrades various dependencies, and introduces support for HostApplicationBuilder across multiple samples and library projects. The changes include conditional compilation using USE_HOST_APPLICATION_BUILDER to allow switching between the new builder and the legacy HostBuilder. A potential issue was identified regarding the removal of ConfigureLogging and ConfigureConfiguration extension methods in AppMixins, which may break the legacy HostBuilder path.
I am having trouble creating individual review comments. Click here to see my feedback.
samples/Dapplo.Hosting.Sample.DefaultWpfDemo/App.xaml.cs (21-52)
The ConfigureLogging and ConfigureConfiguration extension methods were removed from AppMixins. If the App constructor for the !USE_HOST_APPLICATION_BUILDER path relies on these methods, this change will cause a compilation error. Please ensure that the HostBuilder path is still functional or update it accordingly.
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request updates the solution to .NET 10, upgrades various dependencies, and introduces HostApplicationBuilder support across sample projects. Extension methods have been refactored to support generic IHostApplicationBuilder types for better fluent API integration. The review feedback identifies critical logic errors in MSBuild conditions for Visual Studio versions, potential null reference exceptions in path handling, and opportunities to improve code quality through structured logging, proper dependency injection, and the removal of redundant commented-out code.
- Prepare for single file deployment
- Replace positional parameters in structured logging
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request updates the solution to .NET 10.0, including SDK and target framework updates across all projects and samples. It introduces a shared utility project and adds new samples demonstrating the HostApplicationBuilder pattern. Extension methods were refactored to use generics for improved fluent API usage, and logging was updated to a structured format. Feedback primarily focuses on code cleanup, including the removal of redundant commented-out code, empty configuration blocks, and unnecessary UTF-8 BOM characters in project files, as well as ensuring consistent error handling across the new samples.
| if (executableLocation == null) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Silently returning when executableLocation is null is inconsistent with other samples in this pull request, which throw a NotSupportedException. For consistency and to avoid silent failures, it would be better to throw an exception here as well.
if (executableLocation == null)
{
throw new NotSupportedException("Can't start without location.");
}| { | ||
| InitializeComponent(); | ||
| ViewModel = new AppViewModel(); | ||
| ViewModel = AppLocator.Current.GetService<AppViewModel>(); //new AppViewModel(); |
There was a problem hiding this comment.
The commented-out code //new AppViewModel(); can be removed to improve clarity.
ViewModel = AppLocator.Current.GetService<AppViewModel>();| .WithRegistration(locator => { | ||
| // locator.InitializeSplat(); | ||
| }); |
There was a problem hiding this comment.
This .WithRegistration() call has an empty body and contains commented-out code. If it's not needed, it should be removed to simplify the configuration.
| { | ||
| string environmentName = null; | ||
|
|
||
| //var hostSettingsFilePath = System.IO.Path.Combine(Directory.GetCurrentDirectory(), hostSettingsFile); |
There was a problem hiding this comment.
This commented-out line should be removed to improve code clarity.
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| <IsPackable>true</IsPackable> | ||
| <GeneratePackageOnBuild>true</GeneratePackageOnBuild> | ||
| <!--<GeneratePackageOnBuild>true</GeneratePackageOnBuild>--> |
There was a problem hiding this comment.
This commented-out line is now redundant due to the conditional GeneratePackageOnBuild element on the next line. It can be removed to improve code clarity.
| @@ -2,15 +2,19 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <TargetFramework>netstandard2.0</TargetFramework> | |||
There was a problem hiding this comment.
The <TargetFramework> element is redundant as <TargetFrameworks> is also specified. You can remove this line.
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request updates the entire solution to .NET 10.0, including SDK versions in global.json and CI pipelines, and target frameworks across all projects. It introduces comprehensive support for the HostApplicationBuilder API, providing generic extension methods for services like single-instance management, plugin configuration, and UI framework integration (WPF, WinForms, WinUI, Caliburn.Micro, and Metro) to support fluent chaining. Additionally, dependencies such as Caliburn.Micro, MahApps.Metro, and ReactiveUI have been updated to their latest versions. A new common utility project for samples was added, and logging throughout the samples was modernized to use structured logging. Review feedback suggests refining logging property names for clarity, removing dead code in new utility classes, and improving the robustness of JSON configuration parsing.
| builder.WhenNotFirstInstance = (hostingEnvironment, logger) => | ||
| { | ||
| // This is called when an instance was already started, this is in the second instance | ||
| logger.LogWarning("Application {applicationName} already running.", hostingEnvironment.ApplicationName); |
There was a problem hiding this comment.
For consistency with the README and other samples in this repository, consider using the structured logging property name {@ApplicationName} (PascalCase with the destructuring operator).
logger.LogWarning("Application {@ApplicationName} already running.", hostingEnvironment.ApplicationName);| //var hostSettingsFilePath = System.IO.Path.Combine(Directory.GetCurrentDirectory(), hostSettingsFile); | ||
| var hostSettingsFilePath = System.IO.Path.Combine(AppContext.BaseDirectory, hostSettingsFile); | ||
|
|
||
| if (System.IO.File.Exists(hostSettingsFilePath)) | ||
| { | ||
| var jsonData = File.ReadAllText(hostSettingsFilePath); | ||
| var jsonHostSettingsObject = System.Text.Json.Nodes.JsonObject.Parse(jsonData); |
There was a problem hiding this comment.
Consider removing the commented-out code and making the JSON parsing more robust by ensuring the root node is an object before accessing it by key. This prevents potential InvalidOperationException if the JSON root is an array or a primitive value.
var hostSettingsFilePath = System.IO.Path.Combine(AppContext.BaseDirectory, hostSettingsFile);
if (System.IO.File.Exists(hostSettingsFilePath))
{
var jsonData = File.ReadAllText(hostSettingsFilePath);
var jsonHostSettingsObject = System.Text.Json.Nodes.JsonNode.Parse(jsonData) as System.Text.Json.Nodes.JsonObject;| /* | ||
| return AppContext.BaseDirectory; | ||
| return Path.GetDirectoryName(Process.GetCurrentProcess().MainModule.FileName); | ||
| */ | ||
|
|
||
| return AppContext.BaseDirectory; |
There was a problem hiding this comment.
The commented-out code and extra whitespace should be removed to keep the utility class clean and maintainable.
return AppContext.BaseDirectory;| .WithViewsFromAssembly(Assembly.GetExecutingAssembly()) | ||
| .WithRegistration(locator => { | ||
| // locator.InitializeSplat(); | ||
| }); |
There was a problem hiding this comment.
This empty registration block is unnecessary and can be removed to simplify the builder configuration.
.WithViewsFromAssembly(Assembly.GetExecutingAssembly());| /// <param name="instance">some object to fill</param> | ||
| protected override void BuildUp(object instance) => | ||
| this.logger.LogDebug("Should buildup {0}", instance?.GetType().Name); // TODO: don't know how to fill imports yet?//_bootstrapper.Container.InjectProperties(instance); | ||
| this.logger.LogDebug("Should buildup {MemberInfo.Name}", instance?.GetType().Name); // TODO: don't know how to fill imports yet?//_bootstrapper.Container.InjectProperties(instance); |
There was a problem hiding this comment.
The structured logging property name {MemberInfo.Name} is misleading as it suggests a property of a MemberInfo object. Using a more descriptive name like {InstanceType} would improve log clarity and searchability.
this.logger.LogDebug("Should buildup {InstanceType}", instance?.GetType().Name); // TODO: don't know how to fill imports yet?//_bootstrapper.Container.InjectProperties(instance);
|
@Lakritzator |
1 participant
