Open K4PS3 opened 10 months ago
Thank you for your pr.
If you merge in the latest from main it should fix the expired cert issue error when building the features and setup project
The code looks good. I will do a full review when you fix the issue with the build.
Thank you for adding the missing xml comments. I do like the new folder structure for the code it does make it easier to find the classes. Thank you for splitting each class into its own file.
Adding code analysis to the solution is great. I hope it catches most the issues that CodeQl found.
Question when we create the Caliburn.Micro nuget packages do the 2 code analysis packages get added as a dependency?
First of all, sorry for the late response, You are welcome and I hope that my contribution will be useful.
I just pushed commit that i am Confident is building locally with MSBuild without any issues, so The CI should now success.
About your Question, the answer is NO, StyleCop package is Meta package referencing, which in turn referencing stylecop.analyzers.unstable, this package does not contain any runtime files in LIB folder, so there will not be any referencing to it at runtime, the package only contains analyzers.
I glad that you liked changes i made.
By the way, I have been and still do using your framework successfully in many projects, with conjunction with SimpleInjector.
@vb2ae I just pushed another commit and hope it will fix CodeQL issues. The issues are already exist and not introduced by this pr. The 'Generic catch clause' issues still exist and needs manual fix by the framework team.
You can review the code, and give me feedback, so i can fix any issues you find this pr.
Sorry, I forgot to update NameFormat in ViewModelLocatorTests.cs, the issue should be now fixed. CodeQL should be now green with 17 notes about 'Generic catch clause'.
thanks for fixing the codeql errors
@vb2ae If you don't mind, I attached ZIP file Caliburn.Micro.git.zip with proposed changes for '.csproj' to make them more organized.
You can test them locally with 'master' branch of Caliburn.
As you see in the image, the active target framework will be shown in the root of the project, that will make it easier to recognize it. The shared XAML files will shown in its own folder. The shared code of maui shown in Maui folder in the Caliburn.Micro.Maui project, the active target shown in its own folder.
With this, as you can see, the root of the projects less distraction, even with project's nodes expanded.
These changes could be merged with this pr by another commit, and I can move the files from Caliburn.Micro.Platform to the other projects by another commit, so it will be easier to review the changes.
The power of MSBuild can make '.csproj' content smaller, I learned some tricks from the giants of MSBuild, and used here to clean '.csproj' files.
I hope that i am not mixing the things out of the order, and not breaking any rules or guides.
Looking forward for your review and comments.
@K4PS3 thanks
I am intrigued about the .csproj changes looks like they could simplify maintenance of the platform projects by a lot.
@KasperSK are you ok with merging this?
Let me look it over I am not sure that I like the rearranging of the files. I have some time Monday before a flight so I will do it then.
This pull request mainly focused on organizing the core project. You will find descriptive message in each commit.
Here is the main points:
In general the changes made here not critical for the code itself, most of the changes to benefit from the compiler capability, in Roslyn words most of the changes applied to the Trivias.
The missing part that found in
<NoWarn>...</NoWarn>
in .csproj of the core project, These errors needs to be fixed one-by-one with extensive testing and good consideration for the effects on others parts of the framework.I tried to do small changes for each commit to make it easier to review. I also worked on the platform projects, but they need a lot in attention, especially with Conditional compilation.
If this pull request is acceptable, or any part of it can provide any benfit, i will be glade.
I am not native English speaker, so forgive any typos.
The error list
-
AsyncEventHandler.cs
-INotifyPropertyChangedEx.cs
Cause: EventHandler and Ex suffix.
SimpleContainer.cs -> event Action<object> Activated
IActivate.cs -> event AsyncEventHandler<ActivationEventArgs> Activated
IDeactivate.cs -> event AsyncEventHandler<DeactivationEventArgs> Deactivated
Cause: Action instead of EventHandler, custome delegate AsyncEventHandler for EventHandler. Solution: use EventHandler delegate for Action
Screen.cs -> public virtual event AsyncEventHandler<ActivationEventArgs> Activated
Screen.cs -> public virtual event EventHandler<DeactivationEventArgs> AttemptingDeactivation
Screen.cs -> public virtual event AsyncEventHandler<DeactivationEventArgs> Deactivated
ConductorBase.cs -> public virtual event EventHandler<ActivationProcessedEventArgs> ActivationProcessed
PropertyChangedBase.cs -> public virtual event PropertyChangedEventHandler PropertyChanged
Cause: virtual keyword, events should not be overrding. Solution: Remove virtual keyword.
Cause: Nesting public class. Solution: move class to new namespace
Cause: arguments used without validation. Solution: validate arguments of methods, when failed, return, return null or throw.
This is critical issue.
read here and decide when to use ConfigureAwait(false) or ConfigureAwait(true)
ViewAware.cs -> void IViewAware.AttachView(object view, object context)
Screen.cs -> async Task IActivate.ActivateAsync(CancellationToken cancellationToken)
Screen.cs -> async Task IDeactivate.DeactivateAsync(bool close, CancellationToken cancellationToken)
Cause: Explicit implementation of the interface methods. Solution:
This is critical issue.
Cause: catch (Exception ex). Solution: Catch Specific exception type, or rethrow the exception.
ILog.cs -> void Error(...)
PropertyChangedBase.cs -> public virtual bool Set<T>(...)
Cause: using of reserved keyword (Error and Set). Solution: Change the name if possible, otherwise suppress this error.
This is critical issue.
Cause: new task without specifying TaskScheduler. Solution: read the docs and decide.
ResultExtensions.cs -> result.Execute(context ?? new CoroutineExecutionContext())
Cause: There is method named 'ExecuteAsync'. Solution: ignore this error for this case, because calling ExecuteAsync will cause endless loop.
SimpleContainer.cs -> FactoryFactory<T>.Create
Cause: Pure Function not accessing any instance data. Solution: Can't change this since it is used by GetInstance method with reflection, or you have to change the logic of calling this method by reflection.