added_csvfileimportoption_in_consoleapp_v5#665
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds CSV file import functionality to the MSDIAL console application and refactors version number assignment logic across multiple process files.
- Adds ability to import analysis files from CSV format with configurable metadata
- Moves version number assignment to occur before conditional project saving to ensure it's always set
- Improves path handling consistency by using
Path.Combineinstead of hardcoded backslashes
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MSDIAL5/MsdialCoreTestApp/Program.cs | Adds commented example configuration for CSV file import |
| tests/MSDIAL5/MsdialCoreTestApp/Parser/AnalysisFilesParser.cs | Implements CSV parsing functionality with ReadCsvContents and isCsv methods; refactors path handling to use Path.Combine |
| tests/MSDIAL5/MsdialCoreTestApp/Process/LcmsProcess.cs | Moves version number assignment before project save condition |
| tests/MSDIAL5/MsdialCoreTestApp/Process/LcimmsProcess.cs | Moves version number assignment before project save condition |
| tests/MSDIAL5/MsdialCoreTestApp/Process/ImmsProcess.cs | Moves version number assignment before project save condition |
| tests/MSDIAL5/MsdialCoreTestApp/Process/GcmsProcess.cs | Moves version number assignment before project save condition |
| tests/MSDIAL5/MsdialCoreTestApp/Process/DimsProcess.cs | Moves version number assignment before project save condition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AcquisitionType = afAcqType, | ||
| DeconvolutionFilePath = Path.Combine(fileDir, afFilename + "_" + dtString + ".dcl"), | ||
| PeakAreaBeanInformationFilePath = Path.Combine(fileDir, afFilename + "_" + dtString + ".pai"), | ||
| RetentionTimeCorrectionBean = new RetentionTimeCorrectionBean(fileDir + "\\" + afFilename + "_" + dtString + ".rtc"), |
There was a problem hiding this comment.
The ReadCsvContents method contains inconsistent path separator usage. Line 182 uses hardcoded backslash "\\" while lines 180-181 use Path.Combine. For consistency and cross-platform compatibility, line 182 should also use Path.Combine(fileDir, afFilename + "_" + dtString + ".rtc") instead of string concatenation with hardcoded backslash.
| RetentionTimeCorrectionBean = new RetentionTimeCorrectionBean(fileDir + "\\" + afFilename + "_" + dtString + ".rtc"), | |
| RetentionTimeCorrectionBean = new RetentionTimeCorrectionBean(Path.Combine(fileDir, afFilename + "_" + dtString + ".rtc")), |
| // 0 , 1 , 2 , 3 , 4 5 , 6 , 7 | ||
| // "file_path", "file_name", "file_type", "class_id", "acquisition_type", "batch", "analytical_order", "factor" |
There was a problem hiding this comment.
The comment at line 131 contains a typo. The header name for index 5 is listed as "batch" but the actual header name from line 63 is "batch_order". This mismatch could cause confusion when maintaining the code.
| // 0 , 1 , 2 , 3 , 4 5 , 6 , 7 | |
| // "file_path", "file_name", "file_type", "class_id", "acquisition_type", "batch", "analytical_order", "factor" | |
| // 0 , 1 , 2 , 3 , 4 5 , 6 , 7 | |
| // "file_path", "file_name", "file_type", "class_id", "acquisition_type", "batch_order", "analytical_order", "factor" |
| var splitHeader = header.Split(','); | ||
| var headerFields = new List<string>(); | ||
| foreach (var h in splitHeader) | ||
| { | ||
| var h1 = h.ToLowerInvariant(); | ||
| var h2 = h1.Trim(); | ||
| var h3 = h2.Replace(' ', '_'); | ||
| headerFields.Add(h3); | ||
| } | ||
|
|
||
| for (var i = 0; i < searchHeaderNames.Length; i++) | ||
| { | ||
| var name = searchHeaderNames[i]; | ||
| var index = headerFields.IndexOf(name); | ||
| Debug.WriteLine(" Index of {0}: {1}", name, index); | ||
| headerOrder.Add(index); | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| while (!sr.EndOfStream) | ||
| { | ||
| var line = sr.ReadLine(); | ||
| //if (line == null) | ||
| // break; | ||
| // skip lines that start with '#' | ||
| if (line.StartsWith('#'.ToString())) | ||
| continue; | ||
|
|
||
| var fields = line.Split(','); |
There was a problem hiding this comment.
CSV parsing with Split(',') doesn't handle quoted fields containing commas. If a CSV field contains a comma within quotes (e.g., "path,with,commas"), the current implementation will incorrectly split it. Consider using a proper CSV parsing library like CsvHelper or implementing RFC 4180 compliant parsing.
| } | ||
|
|
||
| break; | ||
| } |
There was a problem hiding this comment.
Potential NullReferenceException when CSV file is empty or contains only comment lines. If the file has no valid header line, headerOrder will remain empty and accessing headerOrder[i] at line 109 will throw an ArgumentOutOfRangeException. Consider adding validation after the first while loop to ensure a header was found.
| } | |
| } | |
| // Validate that a valid header was found | |
| if (headerOrder.Count != searchHeaderNames.Length || headerOrder.TrueForAll(idx => idx < 0)) { | |
| throw new InvalidDataException("CSV file does not contain a valid header line with required fields."); | |
| } |
| { | ||
| // 0 , 1 , 2 , 3 , 4 5 , 6 , 7 | ||
| // "file_path", "file_name", "file_type", "class_id", "acquisition_type", "batch", "analytical_order", "factor" | ||
| //var afFilepath = Path.GetFullPath(Path.Combine(line[0], Path.GetDirectoryName(Path.GetFullPath(filepath)))); |
There was a problem hiding this comment.
Potential NullReferenceException when accessing line[0] at line 133. If a CSV row is empty or the file_path column is missing (index < 0 in headerOrder), line[0] will be null, causing Path.GetFullPath(null) to throw an ArgumentNullException. Add validation to check if line[0] is null before using it.
| //var afFilepath = Path.GetFullPath(Path.Combine(line[0], Path.GetDirectoryName(Path.GetFullPath(filepath)))); | |
| //var afFilepath = Path.GetFullPath(Path.Combine(line[0], Path.GetDirectoryName(Path.GetFullPath(filepath)))); | |
| if (line == null || line.Length == 0 || string.IsNullOrWhiteSpace(line[0])) { | |
| throw new ArgumentException("CSV row is empty or missing required file_path column."); | |
| } |
| { | ||
| var csvData = new List<string[]>(); | ||
| // read csv file | ||
| using (var sr = new StreamReader(filepath, System.Text.Encoding.ASCII)) |
There was a problem hiding this comment.
Using System.Text.Encoding.ASCII may cause issues with non-ASCII characters in file paths or metadata. CSV files often contain UTF-8 encoded characters. Consider using System.Text.Encoding.UTF8 instead to handle international characters properly.
| using (var sr = new StreamReader(filepath, System.Text.Encoding.ASCII)) | |
| using (var sr = new StreamReader(filepath, System.Text.Encoding.UTF8)) |
No description provided.