CSV EPIC #149

mnoserat · 2025-09-17T19:02:43Z

No description provided.

- Convert CSVDeliverer and CSVDiscoverer to primary constructors - Add XML documentation with CREF references for CSV classes - Update variable names for consistency - Format code and organize using statements - Create CSVConstants.cs file and update CSV files to use constants

Skyaero42

This needs another pass

Adhere to the single responsibility principle (SRP) use private methods for small tasks, rather than putting everything in one big method.
Consider using private/internal data objects rather than a bunch of variables if they are related. This also allows validation logic to be moved to the object.
Consider using a CSV reading library.
There is hardcoded stuff for generals 1.08 and zero hour 1.04. This is a major issue as these versions are only available when users have applied genpatcher. The official latest versions are 1.05 and 1.09. There is also Generals Online and the SuperHackers pre-release. This needs a redesign.

Skyaero42 · 2025-09-19T20:10:46Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            {
+                _logger.LogError(CsvConstants.CsvUrlMissingLog, discoveredItem?.Id);
+                return OperationResult<ContentManifest>.CreateFailure(CsvConstants.CsvUrlNotProvidedError);
+            }


(line 75-124): Obtaining the csvUrl should be a separate (private) function to adhere to the single responsibility principle

Skyaero42 · 2025-09-19T20:12:40Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            }
+
+            // Determine requested target game (if the discoverer set it)
+            var targetGame = discoveredItem.TargetGame != default ? discoveredItem.TargetGame : (GameType?)null;


Replacing var for GameType? and removing the cast from null increases readability

Skyaero42 · 2025-09-19T20:15:46Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            catch
+            {
+                // Ignore
+            }


Getting the targetGame should be its own method to adhere to SRP.

Skyaero42 · 2025-09-19T20:17:44Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+                {
+                    _logger.LogWarning(CsvConstants.SizeParseFailedWarning, relativePath, lineIndex);
+                    continue;
+                }


Download and parsing the CSV should be a method or methods by it/themselves to adhere to SRP.

Skyaero42 · 2025-09-19T20:17:55Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            using var reader = new StreamReader(stream);
+
+            // Parse CSV streaming
+            var files = new List<ManifestFile>();


Why not use a parser library?

^ I agree with this one, I already mentioned CsvHelper in another comment.

GenHub/GenHub/Features/Content/Services/ContentDiscoverers/CSVDiscoverer.cs

Skyaero42 · 2025-09-19T20:35:12Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+
+                    var fileValid = true;
+
+                    // Check MD5


create private method

Skyaero42 · 2025-09-19T20:35:21Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+                        }
+                    }
+
+                    // Check SHA256


create private method

Skyaero42 · 2025-09-19T20:35:37Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+                        continue;
+                    }
+
+                    var relativePath = columns[CsvConstants.RelativePathColumnIndex].Trim();


Consider using an object

Skyaero42 · 2025-09-19T20:36:03Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+                CurrentOperation = CsvConstants.DownloadingCsvMessage,
+            });
+
+            // CSV download


separate method

Skyaero42 · 2025-09-19T20:42:36Z

Additionally, create a clear title what this PR does, add a (short) description what it does and link the relevant issues (- Resolves #000). The issue linked in the title is an epic and should not be referred to directly.

RMWMStevens · 2025-09-21T20:20:43Z

GenHub/GenHub.Core/Constants/CsvConstants.cs

+namespace GenHub.Core.Constants;
+
+/// <summary>Constants used for CSV file processing and content handling.</summary>
+public static class CsvConstants


Why don't you just use CsvHelper? It takes care of so much of the CSV work for you, then you don't have to remember what the delimiter is, and if the Language is in column 5, etc. You just read the CSV into an object and you're done. Extremely quick & easy, and also much easier to read code-wise & work with from that point forward.

RMWMStevens · 2025-09-21T20:21:01Z

GenHub/GenHub.Core/Constants/CsvConstants.cs

+    public const string FileSystemSourceName = "FileSystem";
+
+    /// <summary>Default hash algorithm to use for file validation.</summary>
+    public const string DefaultHashAlgorithm = "SHA256";


I'm pretty sure this one is already in a constant somewhere.

RMWMStevens · 2025-09-21T20:21:41Z

GenHub/GenHub.Core/Constants/CsvConstants.cs

+    public const string DefaultHashAlgorithm = "SHA256";
+
+    /// <summary>Progress message for CSV download phase.</summary>
+    public const string DownloadingCsvMessage = "Downloading CSV file...";


Personally I think if you're only using this message in a log for example, then you really don't have to turn it into a Constant. I think this is overusing constants a little bit.

RMWMStevens · 2025-09-21T20:25:16Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            try
+            {
+                // Some ContentSearchResult implementations expose a dictionary property named ResolverMetadata
+                // We'll attempt to read it via dynamic/Reflection style if typed property isn't available.


So, is the typed property available? It says "if" it's available.
And is there no easier way of doing this than to use reflection? I feel like often it's needlessly complex and reflection isn't very performant.

RMWMStevens · 2025-09-21T20:28:12Z

GenHub/GenHub/Features/Content/Services/ContentResolvers/CSVResolver.cs

+            using var reader = new StreamReader(stream);
+
+            // Parse CSV streaming
+            var files = new List<ManifestFile>();


^ I agree with this one, I already mentioned CsvHelper in another comment.

RMWMStevens · 2025-09-21T20:30:54Z

GenHub/GenHub/Features/Content/Services/ContentDiscoverers/CSVDiscoverer.cs

+{
+    private readonly ILogger<CSVDiscoverer> _logger = logger;
+    private readonly ManifestDiscoveryService _manifestDiscoveryService = manifestDiscoveryService;
+    private readonly IConfigurationProviderService _configurationProvider = configurationProvider;


Maybe he likes to have all names with underscores, which is not possible directly in the primary constructor. Maybe you can just circumvent this by keeping these lines and adding ?? throw new ArgumentNullException(...);

GenHub/GenHub/Features/Content/Services/ContentDiscoverers/CSVDiscoverer.cs

RMWMStevens · 2025-09-21T20:35:07Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+
+// This class implemented in a very basic way and needs more work
+// It needs to be split into smaller methods
+// Also, the CSV format should be more robustly defined (e.g. using a library like CsvHelper)


Hey, I'm glad to see you mentioning this yourself! That's makes me happy actually, nice job. I'm curious to see what you come up with.

Maybe for next time though: if something needs more work, consider marking the PR as 'draft' until you're happy with it and have addressed all or most of your own bullet points. Until I saw this section, I was under the impression that this PR is "ready to go", but now I think otherwise and feel like you're saying the same thing here. :)

RMWMStevens · 2025-09-21T20:39:37Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+            return OperationResult<ContentManifest>.CreateFailure($"{CsvConstants.ContentNotFoundError}: {contentId}");
+        }
+
+        var result = searchResult.Data!.First();


This code is unsafe. Why not write it like this:

var result = searchResult?.Data?.FirstOrDefault(); var manifest = result?.GetData<ContentManifest>(); return manifest != null ? OperationResult<ContentManifest>.CreateSuccess(manifest) : OperationResult<ContentManifest>.CreateFailure(CsvConstants.ManifestNotAvailableError);

This way an exception will be avoided in every single case a variable here could be null.

RMWMStevens · 2025-09-21T20:40:35Z

GenHub/GenHub/Features/Content/Services/ContentProviders/CSVContentProvider.cs

+        var searchResult = await SearchAsync(query, cancellationToken);
+        if (!searchResult.Success || !searchResult.Data!.Any())
+        {
+            return OperationResult<ContentManifest>.CreateFailure($"{CsvConstants.ContentNotFoundError}: {contentId}");


Why not use the Result.Message or whatever it's called in the response?

…idation fallback - Add CSVDiscoverer and CSVResolver for querying and parsing CSV catalogs - Introduce CsvCatalogEntry model and registry index support for metadata - Enhance GameInstallationValidator with TryCsvValidationAsync fallback - Update ContentSearchQuery with language normalization for CSV filtering - Add LanguageDetector for installation language detection - Integrate CSV components into DI modules (ValidationModule, ContentPipelineModule) - Create comprehensive unit tests for CSV models, discoverer, resolver, and detector - Generate initial CSV files (Generals-1.08.csv, ZeroHour-1.04.csv) and index.json - Add CSV configuration keys and constants for production use Resolves multiple open issues in CSV PR #XX including discovery filtering, language support, manifest generation, and integration with existing validation pipeline.

…ase game manifests for Generals and Zero Hour

…ase game manifests for Generals and Zero Hour (#180)

adding commit

54b13e6

This was referenced Sep 17, 2025

Create CSV Generation Utility #154

Open

Implement Registry Storage Structure #155

Open

Create Index.json Metadata System for CSV registries. #156

Open

Implement Unified Language Support in CSV Pipeline #144

Open

This comment was marked as off-topic.

Sign in to view

Skyaero42 requested changes Sep 19, 2025

View reviewed changes

RMWMStevens reviewed Sep 21, 2025

View reviewed changes

undead2146 added 2 commits September 26, 2025 20:38

chore: merge with main

72ea431

undead2146 changed the title ~~adding file for 108 issue~~ Mega PR for CSV EPIC Oct 16, 2025

undead2146 changed the title ~~Mega PR for CSV EPIC~~ CSV EPIC Oct 19, 2025

undead2146 closed this Oct 21, 2025

undead2146 deleted the feature/CSV-Game-Installation-Validation branch October 21, 2025 21:56

undead2146 restored the feature/CSV-Game-Installation-Validation branch October 22, 2025 10:04

undead2146 reopened this Oct 22, 2025

undead2146 marked this pull request as draft October 22, 2025 10:06

CSVDiscoverer class that implements IContentDiscoverer to discover b…

2f4c357

…ase game manifests for Generals and Zero Hour

undead2146 mentioned this pull request Nov 2, 2025

Refactor(game-client): added detection with hash-based identification and GeneralsOnline support #165

Merged

CSVDiscoverer class that implements IContentDiscoverer to discover b…

4fcb262

…ase game manifests for Generals and Zero Hour (#180)

undead2146 mentioned this pull request Nov 11, 2025

feat: add optional Language filter to ContentSearchQuery #181

Merged

undead2146 closed this Dec 13, 2025

CSV EPIC #149

CSV EPIC #149

Uh oh!

Conversation

mnoserat commented Sep 17, 2025

Uh oh!

This comment was marked as off-topic.

Uh oh!

Skyaero42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skyaero42 commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants