-
Notifications
You must be signed in to change notification settings - Fork 777
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
[WIP] Applying best practice #871
base: master
Are you sure you want to change the base?
Commits on Oct 6, 2023
-
Moved files in Caliburn.Micro.Core project.
The hierarchy of root folders as following: * Framework main types: ----------------------- - Screen - ViewAware - Conductor - Coroutine - Result - EventAggregator * Framework services: --------------------- - PlatformProvider - IoC - Logging * Extended System Types - Types Types shared across The parts of the framework. - Extensions Extension Methods for system types. Inside each folder there maybe one or more of the following folders: - Contracts: The interfaces. - Base: abstract base implementation of the module. - DefaultImpl: Default implementation that can be overriding by Platform. - Impl: Multiple implementation of the contracts that can choosing from. - Models: POCO used as argument for methods. - Extensions: Extension methods for Contracts and main module file. - ExtensionPoints: Service as Static holder that can be opt-in. - EventArgs: POCO for events - EventHandlers: Event Handlers for modules.
Configuration menu - View commit details
-
Copy full SHA for 5e9133e - Browse repository at this point
Copy the full SHA 5e9133eView commit details
Commits on Oct 7, 2023
-
- Extracted types from files containing more than one type.
- Moved contents of TaskExtensions.cs to ResultExtensions.cs, both dealing with IResult. - Deleted TaskExtensions.cs. * Note Nested types still exist in: - WeakValueDictionary contains ValueCollection as private type (OK). - LogManager conatins NullLog as private type [Null Pattern] (OK). - SimpleContainer contains (ContainerEntry and FactoryFactory<T>) as private type (OK). - EventAggregator contains Handler as private type (OK). - Conductor partial class contains Collection as partial class which in turn contains (OneActive and AllActive) (Code Smell), We Should use namespaces. if we were enabling Code Analyzers right now with SDK 7.0.401, we will get CA1052 error among other errors. change global.json SDK version to 7.0.401 and add these properties to the project: <PropertyGroup> <WarningLevel>7</WarningLevel> <TreatWarningsAsErrors>True</TreatWarningsAsErrors> <EnforceCodeStyleInBuild>True</EnforceCodeStyleInBuild> <EnableNETAnalyzers>True</EnableNETAnalyzers> <AnalysisLevel>latest-all</AnalysisLevel> </PropertyGroup>
Configuration menu - View commit details
-
Copy full SHA for e0f7276 - Browse repository at this point
Copy the full SHA e0f7276View commit details -
- Removed region from WeakValueDictionary.cs.
- Code Refactoring applied for : * Use expression body for constructor. * Use expression body for methods. * Use expression body for property. * Use local function. * Use pattern matching * Inline variable declaration. * Inline temporary variable. * Fixing IDE1006 Naming rule violation: Missing prefix:'_'. * Fixing IDE0008 Use explicit type instead of 'var'. * Fixing IDE0059 Unnecessary assignment of value, Use discard * Fixing IDE0003 Name can be simplified, Remove 'this' qualification. * Fixing IDE0016 Null check can be simplified, Use 'throw' expression. * Fixing IDE0034 'default' expression can be simplified. * Fixing IDE0090 'new' expression can be simplified. * Fixing IDE0074 Use compound assignment. * Fixing IDE0044 Make field readonly.
Configuration menu - View commit details
-
Copy full SHA for 81214f5 - Browse repository at this point
Copy the full SHA 81214f5View commit details -
- Splitted long lines for easy read. - Use expression body for lambda expression.
Configuration menu - View commit details
-
Copy full SHA for df3a593 - Browse repository at this point
Copy the full SHA df3a593View commit details -
- Applied 'Return Early Pattern' as much as possible.
- Applied 'Fail Fast Principle' as much as possible. - Minor formatting. The Benefit is the code less nested, The 'Happy path' can hold more code without unnecessary nested complex method, because the 'Return Early Pattern' and 'Fail Fast Principle' paths has deterministic result for known conditions.
Configuration menu - View commit details
-
Copy full SHA for 35fbaac - Browse repository at this point
Copy the full SHA 35fbaacView commit details -
- Added disabled settings for Code Analysis.
You can activate Code Analysis by setting 'SetupCodeAnalysis' to true in the project file '.csproj' and change SDK version in global.json to 7.0.400, build and you will get punch of errors, you suppress them by setting 'SuppressCodeAnalysisWarnings' to true. The list of errors in the .csproj file need to be fixed next, one by one, we remove item from NoWarn, fix the code, build, test and refactor.
Configuration menu - View commit details
-
Copy full SHA for ee63687 - Browse repository at this point
Copy the full SHA ee63687View commit details -
- Enabled Code Analysis, 6 types of issues fixed, another 14 types st…
…ill there and need attention, some of theme is critical. CA1711 // Identifiers should not have incorrect suffix - AsyncEventHandler.cs - INotifyPropertyChangedEx.cs Cause: EventHandler and Ex suffix. CA1003 // Use generic event handler instances - 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<object>, CA1070 // Do not declare event fields as virtual - 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. CA1052 // Static holder types should be Static or NotInheritable - ConductorWithCollectionAllActive.cs -> Collection::AllActive - ConductorWithCollectionOneActive.cs -> Collection::OneActive CA1034 // Nested types should not be visible - ConductorWithCollectionAllActive.cs -> Collection::AllActive - ConductorWithCollectionOneActive.cs -> Collection::OneActive Cause: Nesting public class. Solution: move class to new namespace namespace Caliburn.Micro.Conductor.Collection { public class AllActive<T> : ConductorBase<T> { } } namespace Caliburn.Micro.Conductor.Collection { public class OneActive<T> : ConductorBase<T> { } } CA1062 // Validate arguments of public methods Cause: arguments used without validation. Solution: validate arguments of methods, when failed, return, return null or throw. - throw new ArgumentNullException(nameof(argumentName)) - throw new ArgumentException(message, nameof(argumentName)) CA2007 // Consider calling ConfigureAwait on the awaited task This is critical issue. read here and decide when to use ConfigureAwait(false) or ConfigureAwait(true) https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2007 CA1033 // Interface methods should be callable by child types - 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: - Add public method with same signature (Will not cause breaking change). - Make class sealed (Breaking change). - Make the implementation public instead of Explicit. CA1031 // Do not catch general exception types This is critical issue. Cause: catch (Exception ex). Solution: Catch Specific exception type, or rethrow the exception. CA1716 // Identifiers should not match keywords - 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. CA2008 // Do not create tasks without passing a TaskScheduler This is critical issue. - DefaultPlatformProvider.cs -> public virtual Task OnUIThreadAsync(Func<Task> action) => Task.Factory.StartNew(action); Cause: new task without specifying TaskScheduler. Solution: read the docs and decid. CA1849 // Call async methods when in an async method - 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. CA1822 // Mark members as static CA1812 // Avoid uninstantiated internal classes - 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.
Configuration menu - View commit details
-
Copy full SHA for 44a7f54 - Browse repository at this point
Copy the full SHA 44a7f54View commit details -
- Added StyleCop package. - Added StyleCop configuration file 'stylecop.json'. - Applied StyleCop rules, Methods, Properties, Events and Indexers ordered according to these rules. - Removed empty xml documentation. - Added missing curly braces. - .editorconfig changed to enforce specific coding style, these changes can be rolled-back and StyleCop will enforce the new changes. By all these changes you should now have well structured project, organized by features, each feature folder contains all necessary files. Coding style enforced by dotnet analyzers and StyleCop, The rules is configurable via .editorconfig and stylecop.json files. But there still a lot of improvements that can be applied to the code itself by using new capabilities of the compiler, for example using record and struct record. The important final note, the warnings in (<NoWarn>...</NoWarn>) have to be resolved, they are critical and change the framework behaviors.
Configuration menu - View commit details
-
Copy full SHA for bd1b3a9 - Browse repository at this point
Copy the full SHA bd1b3a9View commit details -
Added the missing xml documentation because previous commits failed t…
…o build because of them.
Configuration menu - View commit details
-
Copy full SHA for 53b2f1d - Browse repository at this point
Copy the full SHA 53b2f1dView commit details
Commits on Oct 9, 2023
-
- Refactored all (.csproj) to be more clean and clear.
- Added standard text for missing xml comments. - The code Refactored according to the configured analyzers. - Added comments to '.editorconfig' to explain the changed diagnostic from silent or suggestion to warning. - All the projects now build with all analyzers enabled including StyleCop. - The tests passing. - There is still issues needs to be resolved, for now they suppressed in 'Directory.Build.props', You will find note there with all errors. - Test files in Caliburn.Micro.Core.Tests are moved to sub folder and extracted new files, so files contains single type. There still a lot places that can be improved.
Configuration menu - View commit details
-
Copy full SHA for ee08a04 - Browse repository at this point
Copy the full SHA ee08a04View commit details -
This commit should fix most of the CodeQL errors and warning.
- Changed the wrong format of the string 'NameFormat' in the test prohject. - Merged nested 'if' statements. - Used LINQ to avoid temporary capturing and mutating 'for' variable. - Removed unnecessary casting. - Changed the equality comparison from '==' to 'Equals'. Of course 'Generic catch clause' errors needs to be fixed carefully and take note that the fix will cause Breaking changes for the framework.
Configuration menu - View commit details
-
Copy full SHA for 392c420 - Browse repository at this point
Copy the full SHA 392c420View commit details -
Configuration menu - View commit details
-
Copy full SHA for f7bda46 - Browse repository at this point
Copy the full SHA f7bda46View commit details -
- Fixed NameFormat issue in ViewModelLocatorTests.cs.
CodeQL should be now green with 17 notes about 'Generic catch clause'
Configuration menu - View commit details
-
Copy full SHA for 0ad7002 - Browse repository at this point
Copy the full SHA 0ad7002View commit details
Commits on Oct 12, 2023
-
- Fixed a bug (parent is null), I introduced when CodeQL warnings was…
… fixed. - Tested locally and working correctly. The code now more readable and clear in its purpose.
Configuration menu - View commit details
-
Copy full SHA for b4e9b39 - Browse repository at this point
Copy the full SHA b4e9b39View commit details
Commits on Oct 14, 2023
-
- Fixed another bug that I introduced when CodeQL fixed (NullReferenc…
…eException, handler is null). I reviewed all the changes related to (someVariable.Equals(someValue)), all of them should be ok.
Configuration menu - View commit details
-
Copy full SHA for 05e4ab4 - Browse repository at this point
Copy the full SHA 05e4ab4View commit details