Merge pull request #370 from Sewer56/module-issue-v2-improvements

Added: Extra ModuleIssueV2 Improvements

src/Bannerlord.ModuleManager.Models/ApplicationVersion.cs

@@ -62,7 +62,14 @@ public bool IsSame(ApplicationVersion? other) =>
     public bool IsSameWithChangeSet(ApplicationVersion? other) =>
         Major == other?.Major && Minor == other.Minor && Revision == other.Revision && ChangeSet == other.ChangeSet;
 
-    public override string ToString() => $"{GetPrefix(ApplicationVersionType)}{Major}.{Minor}.{Revision}.{ChangeSet}";
+    public override string ToString()
+    {
+        // Most user mods skip this, so to be user-friendly we can omit it if it was not originally specified.
+        return ChangeSet == 0 
+            ? $"{GetPrefix(ApplicationVersionType)}{Major}.{Minor}.{Revision}" 
+            : $"{GetPrefix(ApplicationVersionType)}{Major}.{Minor}.{Revision}.{ChangeSet}";
+    }
+    public string ToStringWithoutChangeset() => $"{GetPrefix(ApplicationVersionType)}{Major}.{Minor}.{Revision}";
 
     public int CompareTo(ApplicationVersion? other) => ApplicationVersionComparer.CompareStandard(this, other);
 

src/Bannerlord.ModuleManager.Models/ApplicationVersionRange.cs

@@ -54,7 +54,16 @@ public bool IsSame(ApplicationVersionRange? other) =>
     public bool IsSameWithChangeSet(ApplicationVersionRange? other) =>
         Min.IsSameWithChangeSet(other?.Min) && Max.IsSameWithChangeSet(other?.Max);
 
-    public override string ToString() => $"{Min} - {Max}";
+    public override string ToString() 
+    {
+        // If the min and max changeset are at their default (0 and i32::MAX), we can
+        // simplify how we print the version. End user mods rarely use the changeset,
+        // it's only really used in official packages.
+        if (Min.ChangeSet == 0 && Max.ChangeSet == int.MaxValue)
+            return $"{Min.ToStringWithoutChangeset()} - {Max.ToStringWithoutChangeset()}";
+
+        return $"{Min} - {Max}";
+    }
 
     public static bool TryParse(string versionRangeAsString, out ApplicationVersionRange versionRange)
     {

src/Bannerlord.ModuleManager.Models/Issues/ModuleIssueV2.cs

@@ -326,44 +326,44 @@ ApplicationVersionRange VersionRange
 ) : ModuleVersionMismatchIssue(Module, Dependency);
 
 /// <summary>
-///     Represents an issue where a dependency's version is higher than the maximum allowed specific version.
-///     This occurs when a dependency module's version exceeds an exact version requirement.
+///     Represents an issue where a dependency's version is lower than the minimum allowed specific version.
+///     This occurs when a dependency module's version is below an version requirement.
 /// </summary>
 /// <param name="Module">The module with the version constraint</param>
-/// <param name="Dependency">The dependency module that exceeds the version requirement</param>
+/// <param name="Dependency">The dependency module that is under the version requirement</param>
 /// <param name="Version">The specific version that should not be exceeded</param>
 /// <remarks>
 /// This issue occurs when a module specifies incompatible versions.
 /// 
 /// Example scenario:
 /// ```xml
 /// <Module>
-///     <!-- 👇 Current mod is `BetterSiege` -->
-///     <Id value="BetterSiege"/>
+///     <!-- 👇 Current mod is `Better Sieges` with id `BetterSieges` -->
+///     <Id value="BetterSieges"/>
+///     <Name value="Better Sieges"/>
 ///     <DependedModuleMetadatas>
-///         <!-- ✅ `Bannerlord.Harmony` is installed -->
-///         <DependedModuleMetadata id="Bannerlord.Harmony" order="LoadBeforeThis" version="v2.2.2" />
-///         <!-- ❌ However the installed version of `Bannerlord.Harmony` (`v2.3.0`)
-///                 is greater than requested version `v2.2.2` -->
+///         <!-- 💡 The required mod `Harmony` has id `Bannerlord.Harmony` -->
+///         <!-- ❌ `Bannerlord.Harmony` version `v2.2.2` is older than minimum allowed version `v2.3.0` -->
+///         <DependedModuleMetadata id="Bannerlord.Harmony" version="v2.3.0" />
 ///     </DependedModuleMetadatas>
 /// </Module>
 /// ```
 /// 
-/// If a higher version of Harmony (e.g., `v2.3.0`) is installed than allowed, this issue will be raised.
+/// If a higher or equal version of Harmony (e.g., `v2.3.0`) is installed than allowed, this issue will be solved.
 /// </remarks>
 #if !BANNERLORDBUTRMODULEMANAGER_PUBLIC
 internal
 #else
 public
 # endif
-    sealed record ModuleVersionMismatchLessThanOrEqualSpecificIssue(
+    sealed record ModuleVersionTooLowIssue(
     ModuleInfoExtended Module,
     ModuleInfoExtended Dependency,
     ApplicationVersion Version
 ) : ModuleVersionMismatchSpecificIssue(Module, Dependency, Version)
 {
     public override string ToString() => 
-        $"The module '{Module.Id}' requires version {Version} or lower of '{Dependency.Id}', but version {Dependency.Version} is installed";
+        $"The module '{Module.Id}' requires version {Version} or higher of '{Dependency.Id}', but version {Dependency.Version} is installed";
 
     public override LegacyModuleIssue ToLegacy() => new(Module, Dependency.Id, ModuleIssueType.VersionMismatchLessThanOrEqual, ToString(), new ApplicationVersionRange(Version, Version));
 }
@@ -456,7 +456,7 @@ public override string ToString() =>
 ///     This occurs when one module explicitly declares it cannot work with another module.
 /// </summary>
 /// <param name="Module">The module that has declared an incompatibility</param>
-/// <param name="IncompatibleModuleId">The ID of the module that is incompatible with the target</param>
+/// <param name="IncompatibleModule">The module that is incompatible with the target</param>
 /// <remarks>
 /// This issue occurs when a module explicitly marks another module as incompatible.
 /// 
@@ -480,11 +480,11 @@ public override string ToString() =>
 # endif
     sealed record ModuleIncompatibleIssue(
     ModuleInfoExtended Module,
-    string IncompatibleModuleId
+    ModuleInfoExtended IncompatibleModule
 ) : ModuleIssueV2(Module)
 {
-    public override string ToString() => $"'{IncompatibleModuleId}' is incompatible with this module";
-    public override LegacyModuleIssue ToLegacy() => new(Module, IncompatibleModuleId, ModuleIssueType.Incompatible, ToString(), ApplicationVersionRange.Empty);
+    public override string ToString() => $"'{IncompatibleModule.Id}' is incompatible with this module";
+    public override LegacyModuleIssue ToLegacy() => new(Module, IncompatibleModule.Id, ModuleIssueType.Incompatible, ToString(), ApplicationVersionRange.Empty);
 }
 
 /// <summary>
@@ -610,7 +610,7 @@ DependentModuleMetadata ConflictingModule
 # endif
     sealed record ModuleDependencyConflictCircularIssue(
     ModuleInfoExtended Module,
-    DependentModuleMetadata CircularDependency
+    ModuleInfoExtended CircularDependency
 ) : ModuleIssueV2(Module)
 {
     public override string ToString() => $"Module '{Module.Id}' and '{CircularDependency.Id}' have circular dependencies";
@@ -648,11 +648,11 @@ DependentModuleMetadata CircularDependency
 # endif
     sealed record ModuleDependencyNotLoadedBeforeIssue(
     ModuleInfoExtended Module,
-    string DependencyId
+    DependentModuleMetadata Dependency
 ) : ModuleIssueV2(Module)
 {
-    public override string ToString() => $"'{DependencyId}' should be loaded before '{Module.Id}'";
-    public override LegacyModuleIssue ToLegacy() => new(Module, DependencyId, ModuleIssueType.DependencyNotLoadedBeforeThis, ToString(), ApplicationVersionRange.Empty);
+    public override string ToString() => $"'{Dependency.Id}' should be loaded before '{Module.Id}'";
+    public override LegacyModuleIssue ToLegacy() => new(Module, Dependency.Id, ModuleIssueType.DependencyNotLoadedBeforeThis, ToString(), ApplicationVersionRange.Empty);
 }
 
 /// <summary>
@@ -686,11 +686,11 @@ string DependencyId
 # endif
     sealed record ModuleDependencyNotLoadedAfterIssue(
     ModuleInfoExtended Module,
-    string DependencyId
+    DependentModuleMetadata Dependency
 ) : ModuleIssueV2(Module)
 {
-    public override string ToString() => $"'{DependencyId}' should be loaded after '{Module.Id}'";
-    public override LegacyModuleIssue ToLegacy() => new(Module, DependencyId, ModuleIssueType.DependencyNotLoadedAfterThis, ToString(), ApplicationVersionRange.Empty);
+    public override string ToString() => $"'{Dependency.Id}' should be loaded after '{Module.Id}'";
+    public override LegacyModuleIssue ToLegacy() => new(Module, Dependency.Id, ModuleIssueType.DependencyNotLoadedAfterThis, ToString(), ApplicationVersionRange.Empty);
 }
 
 /// <summary>
@@ -773,8 +773,8 @@ ModuleInfoExtended Module
 ///     <!-- 👇 Current mod is `ImprovedGarrisons` -->
 ///     <Id value="ImprovedGarrisons"/>
 ///     <DependedModules>
-///         <!-- ❌ Empty/invalid dependency entry -->
-///         <DependedModule />
+///         <!-- ❌ Empty/invalid dependency entry (empty id) -->
+///         <DependedModule id="" />
 ///         <!-- 💡 Consider adding an `id` field
 ///              <DependedModuleMetadata id="GarrisonsExtensions" />
 ///         -->

src/Bannerlord.ModuleManager/ModuleUtilities.cs

@@ -263,12 +263,14 @@ public static IEnumerable<ModuleIssueV2> ValidateModuleEx(
     /// <param name="targetModule">The module to validate</param>
     /// <param name="isSelected">Function that determines if a module is enabled</param>
     /// <param name="isValid">Function that determines if a module is valid</param>
+    /// <param name="validateDependencies">Set this to true to also report errors in the target module's dependencies. e.g. Missing dependencies of dependencies.</param>
     /// <returns>Any errors that were detected during inspection</returns>
     public static IEnumerable<ModuleIssueV2> ValidateModuleEx(
         IReadOnlyList<ModuleInfoExtended> modules,
         ModuleInfoExtended targetModule,
         Func<ModuleInfoExtended, bool> isSelected,
-        Func<ModuleInfoExtended, bool> isValid)
+        Func<ModuleInfoExtended, bool> isValid,
+        bool validateDependencies = true)
     {
         var visited = new HashSet<ModuleInfoExtended>();
         foreach (var issue in ValidateModuleEx(modules, targetModule, visited, isSelected, isValid))
@@ -287,7 +289,7 @@ public static IEnumerable<ModuleIssueV2> ValidateModuleEx(
     /// <param name="isValid">Function that determines if a module is valid</param>
     /// <param name="validateDependencies">Set this to true to also report errors in the target module's dependencies. e.g. Missing dependencies of dependencies.</param>
     /// <returns>Any errors that were detected during inspection</returns>
-    private static IEnumerable<ModuleIssueV2> ValidateModuleEx(
+    public static IEnumerable<ModuleIssueV2> ValidateModuleEx(
         IReadOnlyList<ModuleInfoExtended> modules,
         ModuleInfoExtended targetModule,
         HashSet<ModuleInfoExtended> visitedModules,
@@ -386,8 +388,12 @@ public static IEnumerable<ModuleIssueV2> ValidateModuleDependenciesDeclarationsE
                     .Where(x => x.LoadType != LoadType.None)
                     .FirstOrDefault(x => string.Equals(x.Id, targetModule.Id, StringComparison.Ordinal)) is { } metadata)
             {
-                if (metadata.LoadType == module.LoadType)
-                    yield return new ModuleDependencyConflictCircularIssue(targetModule, metadata);
+                if (metadata.LoadType != module.LoadType) 
+                    continue;
+
+                // Find the full module with given ID.
+                var fullModule = modules.First(x => moduleInfo.Id == x.Id);
+                yield return new ModuleDependencyConflictCircularIssue(targetModule, fullModule);
             }
         }
     }
@@ -477,7 +483,7 @@ private static IEnumerable<ModuleIssueV2> ValidateModuleDependenciesEx(
                 // dependedModuleMetadata.Version > dependedModule.Version
                 if (!metadata.IsOptional && (ApplicationVersionComparer.CompareStandard(metadata.Version, metadataModule.Version) > 0))
                 {
-                    yield return new ModuleVersionMismatchLessThanOrEqualSpecificIssue(
+                    yield return new ModuleVersionTooLowIssue(
                         targetModule,
                         metadataModule,
                         metadata.Version);
@@ -517,7 +523,7 @@ private static IEnumerable<ModuleIssueV2> ValidateModuleDependenciesEx(
 
             // If the incompatible mod is selected, this mod should be disabled
             if (isSelected(metadataModule))
-                yield return new ModuleIncompatibleIssue(targetModule, metadataModule.Id);
+                yield return new ModuleIncompatibleIssue(targetModule, metadataModule);
         }
 
         // If another mod declared incompatibility and is selected, disable this
@@ -535,7 +541,7 @@ private static IEnumerable<ModuleIssueV2> ValidateModuleDependenciesEx(
 
                     // If the incompatible mod is selected, this mod is disabled
                     if (isSelected(module))
-                        yield return new ModuleIncompatibleIssue(targetModule, module.Id);
+                        yield return new ModuleIncompatibleIssue(targetModule, module);
                 }
             }
         }
@@ -603,14 +609,15 @@ public static IEnumerable<ModuleIssueV2> ValidateLoadOrderEx(
                 continue;
             }
 
+            // TODO(sewer): Return full module here later. Right now I don't have a good way to test some assertions.
             if (metadata.LoadType == LoadType.LoadBeforeThis && metadataIdx > targetModuleIdx)
             {
-                yield return new ModuleDependencyNotLoadedBeforeIssue(targetModule, metadata.Id);
+                yield return new ModuleDependencyNotLoadedBeforeIssue(targetModule, metadata);
             }
 
             if (metadata.LoadType == LoadType.LoadAfterThis && metadataIdx < targetModuleIdx)
             {
-                yield return new ModuleDependencyNotLoadedAfterIssue(targetModule, metadata.Id);
+                yield return new ModuleDependencyNotLoadedAfterIssue(targetModule, metadata);
             }
         }
     }