When the sheet name is specified as $Property$, it auto splits into multiple sheets if the property is enumerable, Supports template for nested objects #959
Conversation
{{Funds.Id}} {{Funds.Name}} {{Funds.Identity.Id}} {{Funds.Identity.Type}} {{Funds.SetupDate.Year}}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-sheet expansion capabilities and enhanced object flattening with circular reference protection to the OpenXml template engine. The reviewer identified several critical bugs, including a double increment of the sheet index that breaks file relationships and a loop termination issue that prevents processing multiple template sheets. Additionally, feedback was provided regarding redundant type checks, inefficient regex instantiation, unnecessary decimal parsing, and missing namespace imports required for cross-platform compilation.
| sheetIdx++; | ||
| var newSheetPath = $"xl/worksheets/sheet{sheetIdx}.xml"; | ||
|
|
||
| // 处理表名 | ||
| string finalSheetName; | ||
| if (subValues.TryGetValue("SheetName", out var customSheetName) && customSheetName != null) | ||
| { | ||
| finalSheetName = customSheetName.ToString()?.Trim() ?? $"{baseSheetName}{subIndex++}"; | ||
| } | ||
| else | ||
| { | ||
| finalSheetName = $"{baseSheetName}{sheetIdx++}"; | ||
| } |
There was a problem hiding this comment.
The sheetIdx variable is incremented twice within the loop: once at line 184 for the file path and again at line 195 for the default sheet name. This results in a mismatch between the generated XML file names (e.g., sheet1.xml) and the indices stored in allSheetInfos (e.g., 2), which will cause Excel to fail when opening the file because the relationship targets in workbook.xml will point to non-existent files. Additionally, subIndex is defined but not used in the default naming logic.
sheetIdx++;
var newSheetPath = $"xl/worksheets/sheet{sheetIdx}.xml";
// 处理表名
string finalSheetName;
if (subValues.TryGetValue("SheetName", out var customSheetName) && customSheetName != null)
{
finalSheetName = customSheetName.ToString()?.Trim() ?? $"{baseSheetName}{subIndex++}";
}
else
{
finalSheetName = $"{baseSheetName}{subIndex++}";
}| if (value == null || value.GetType() is not Type type) | ||
| { | ||
| replacements[key] = string.Empty; | ||
| return; | ||
| } |
There was a problem hiding this comment.
| using System.ComponentModel; | ||
| using System.Xml.Linq; |
There was a problem hiding this comment.
This new file is missing several required using statements for types used in the code, such as System.Text.RegularExpressions (for Regex), System.Reflection (for PropertyInfo), System.Collections (for IEnumerable), System.Globalization (for CultureInfo), and System.Runtime.CompilerServices (for RuntimeHelpers). Since the project targets multiple frameworks including those without global usings, these should be explicitly imported to ensure compilation across all targets.
|
|
||
| private async Task<bool> HookSheetProcess(OpenXmlZip outputFileArchive, string realSheetName, IDictionary<int, string> templateSharedStrings, int sheetIdx, List<(int Index, string Name)> allSheetInfos, ZipArchiveEntry templateSheet, string templateFullName, IDictionary<string, object?> inputValues, CancellationToken cancellationToken) | ||
| { | ||
| var m = Regex.Match(realSheetName, @"\$([^$]+)\$"); |
There was a problem hiding this comment.
Creating a new Regex instance on every call to HookSheetProcess is inefficient. Since this is a constant pattern, it should be defined as a static compiled field. Additionally, realSheetName should be checked for null before calling Regex.Match to avoid a ArgumentNullException if the sheet name mapping fails.
| if (TypeHelper.IsNumericType(type)) | ||
| { | ||
| if (decimal.TryParse(cellValueStr, out var decimalValue)) | ||
| cellValueStr = decimalValue.ToString(CultureInfo.InvariantCulture); | ||
| } |
There was a problem hiding this comment.
Parsing cellValueStr back to a decimal just to call ToString(CultureInfo.InvariantCulture) is redundant and inefficient. cellValueStr was already obtained from the object. If the goal is to ensure invariant culture formatting for numeric types, it should be applied directly when converting the original numeric value to a string.
| if (await HookSheetProcess(outputFileArchive, realSheetName, templateSharedStrings, sheetIdx, allSheetInfos, templateSheet, templateFullName, inputValues, cancellationToken).ConfigureAwait(false)) | ||
| break; |
There was a problem hiding this comment.
Breaking the loop when HookSheetProcess returns true prevents any subsequent template sheets from being processed. If a template contains multiple sheets and one of them uses the expansion logic, all sheets following it will be missing from the output file. Furthermore, sheetIdx is passed by value, so the caller's index is not updated after expansion, which would cause file name conflicts if the loop were to continue.
|
Thank you for your contribution, I will try to evaluate it as soon as possible. Please change the title and description to include a more detailed description of the intents of the PR. |
with template
model
`
public record struct Identity(int Type, string Id);
`
with data
specify sheetname by SheetName property, if not exist set as what in $$ plus 1,2,3
result


