Skip to content

Commit dd35e0f

Browse files
jbrinkmanjbrinkman
committed
chore: address review feedback (validation aggregation, template fix, accessible toggles, robust JS)
Signed-off-by: jbrinkman <[email protected]>
1 parent 4f06dbf commit dd35e0f

File tree

5 files changed

+36
-36
lines changed

5 files changed

+36
-36
lines changed

‎src/DotNetApiDiff/ExitCodes/ExitCodeManager.cs‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,8 @@ public int GetExitCode(ApiComparison apiComparison)
134134
// Validate the comparison result
135135
if (!apiComparison.IsValid())
136136
{
137-
var errorMessage = string.IsNullOrEmpty(apiComparison.InvalidMessage)
138-
? "API comparison is invalid"
137+
var errorMessage = string.IsNullOrEmpty(apiComparison.InvalidMessage)
138+
? "API comparison is invalid"
139139
: $"API comparison is invalid: {apiComparison.InvalidMessage}";
140140
_logger?.LogWarning("{ErrorMessage}, returning ComparisonError exit code", errorMessage);
141141
return ComparisonError;
@@ -180,10 +180,7 @@ public int GetExitCodeForException(Exception exception)
180180
_ => UnexpectedError
181181
};
182182

183-
_logger?.LogWarning(
184-
"Exception of type {ExceptionType} occurred, returning {ExitCode} exit code",
185-
exception.GetType().Name,
186-
GetExitCodeDescription(exitCode));
183+
_logger?.LogWarning("Exception of type {ExceptionType} occurred, returning {ExitCode} exit code", exception.GetType().Name, GetExitCodeDescription(exitCode));
187184

188185
return exitCode;
189186
}

‎src/DotNetApiDiff/Models/ApiComparison.cs‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -71,44 +71,47 @@ public bool IsValid()
7171
// Clear any previous validation message
7272
InvalidMessage = string.Empty;
7373

74+
var errors = new List<string>();
75+
7476
// Validate all changes
7577
var allChanges = AllChanges.ToList();
7678
var invalidChanges = allChanges.Where(c => !c.IsValid()).ToList();
7779
if (invalidChanges.Any())
7880
{
79-
InvalidMessage = $"Found {invalidChanges.Count} invalid change(s): {string.Join(", ", invalidChanges.Select(c => $"'{c.Description}' ({c.Type})"))}";
80-
return false;
81+
errors.Add($"Found {invalidChanges.Count} invalid change(s): {string.Join(", ", invalidChanges.Select(c => $"'{c.Description}' ({c.Type})"))}");
8182
}
8283

8384
// Validate that additions only contain Added changes
8485
var wrongAdditions = Additions.Where(c => c.Type != ChangeType.Added).ToList();
8586
if (wrongAdditions.Any())
8687
{
87-
InvalidMessage = $"Additions collection contains {wrongAdditions.Count} change(s) with wrong type: {string.Join(", ", wrongAdditions.Select(c => $"'{c.Description}' ({c.Type})"))}";
88-
return false;
88+
errors.Add($"Additions collection contains {wrongAdditions.Count} change(s) with wrong type: {string.Join(", ", wrongAdditions.Select(c => $"'{c.Description}' ({c.Type})"))}");
8989
}
9090

9191
// Validate that removals only contain Removed changes
9292
var wrongRemovals = Removals.Where(c => c.Type != ChangeType.Removed).ToList();
9393
if (wrongRemovals.Any())
9494
{
95-
InvalidMessage = $"Removals collection contains {wrongRemovals.Count} change(s) with wrong type: {string.Join(", ", wrongRemovals.Select(c => $"'{c.Description}' ({c.Type})"))}";
96-
return false;
95+
errors.Add($"Removals collection contains {wrongRemovals.Count} change(s) with wrong type: {string.Join(", ", wrongRemovals.Select(c => $"'{c.Description}' ({c.Type})"))}");
9796
}
9897

9998
// Validate that modifications only contain Modified changes
10099
var wrongModifications = Modifications.Where(c => c.Type != ChangeType.Modified).ToList();
101100
if (wrongModifications.Any())
102101
{
103-
InvalidMessage = $"Modifications collection contains {wrongModifications.Count} change(s) with wrong type: {string.Join(", ", wrongModifications.Select(c => $"'{c.Description}' ({c.Type})"))}";
104-
return false;
102+
errors.Add($"Modifications collection contains {wrongModifications.Count} change(s) with wrong type: {string.Join(", ", wrongModifications.Select(c => $"'{c.Description}' ({c.Type})"))}");
105103
}
106104

107105
// Validate that excluded only contain Excluded changes
108106
var wrongExcluded = Excluded.Where(c => c.Type != ChangeType.Excluded).ToList();
109107
if (wrongExcluded.Any())
110108
{
111-
InvalidMessage = $"Excluded collection contains {wrongExcluded.Count} change(s) with wrong type: {string.Join(", ", wrongExcluded.Select(c => $"'{c.Description}' ({c.Type})"))}";
109+
errors.Add($"Excluded collection contains {wrongExcluded.Count} change(s) with wrong type: {string.Join(", ", wrongExcluded.Select(c => $"'{c.Description}' ({c.Type})"))}");
110+
}
111+
112+
if (errors.Count > 0)
113+
{
114+
InvalidMessage = string.Join("; ", errors);
112115
return false;
113116
}
114117

‎src/DotNetApiDiff/Reporting/HtmlTemplates/change-group.scriban‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
<div class="type-changes">
1616
<div class="category-changes">
1717
{{for change in group.changes}}
18-
<div class="change-item {{ change_type }}">
18+
<div class="change-item {{ change.change_type }}">
1919
<div class="change-header">
2020
<div class="change-name">
2121
<span class="element-type">{{ change.element_type }}</span>

‎src/DotNetApiDiff/Reporting/HtmlTemplates/main-layout.scriban‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,8 +151,8 @@
151151
{{end}}
152152
<!-- Type groups within this change category -->
153153
{{for group in section.grouped_changes}}
154-
<div class="type-group">
155-
<div class="type-header" onclick="toggleTypeGroup('{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}')">
154+
<div class="type-group" data-type-group-id="{{ section.change_type }}-{{ group.key | string.replace " " "-" | string.replace "." "-" | string.replace "`" "-" }}">
155+
<div class="type-header">
156156
<h3 class="type-name">
157157
📁 {{ group.key }}
158158
<span class="type-summary">
@@ -163,7 +163,7 @@
163163
{{if group.has_breaking_changes}}
164164
<span class="breaking-type-badge">⚠️ Breaking Changes</span>
165165
{{end}}
166-
<button class="type-toggle collapsed">
166+
<button class="type-toggle collapsed" onclick="toggleTypeGroup(this.closest('.type-group').dataset.typeGroupId)">
167167
<span class="toggle-icon">▶</span>
168168
</button>
169169
</div>

‎src/DotNetApiDiff/Reporting/HtmlTemplates/scripts.js‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
function toggleSignature(detailsId) {
22
const details = document.getElementById(detailsId);
3-
const button = details.previousElementSibling.querySelector('.toggle-btn');
4-
const icon = button.querySelector('.toggle-icon');
3+
const button = details?.previousElementSibling?.querySelector('.toggle-btn');
4+
const icon = button?.querySelector('.toggle-icon');
5+
6+
if (!details || !button || !icon) return;
57

68
if (details.style.display === 'none') {
79
details.style.display = 'block';
@@ -17,8 +19,10 @@ function toggleSignature(detailsId) {
1719
function toggleConfig() {
1820
const details = document.getElementById('config-details');
1921
const button = document.querySelector('.toggle-button');
20-
const icon = button.querySelector('.toggle-icon');
21-
const text = button.querySelector('.toggle-text');
22+
const icon = button?.querySelector('.toggle-icon');
23+
const text = button?.querySelector('.toggle-text');
24+
25+
if (!details || !button || !icon || !text) return;
2226

2327
if (details.style.display === 'none') {
2428
details.style.display = 'block';
@@ -52,8 +56,10 @@ function getSectionState(sectionId) {
5256
function toggleSection(sectionId) {
5357
const section = document.getElementById(sectionId);
5458
const content = document.getElementById(sectionId + '-content');
55-
const button = section.querySelector('.section-toggle');
56-
const icon = button.querySelector('.toggle-icon');
59+
const button = section?.querySelector('.section-toggle');
60+
const icon = button?.querySelector('.toggle-icon');
61+
62+
if (!section || !content || !button || !icon) return;
5763

5864
if (content.classList.contains('collapsed')) {
5965
// Expand section
@@ -73,10 +79,11 @@ function toggleSection(sectionId) {
7379
// Toggle type group functionality
7480
function toggleTypeGroup(typeGroupId) {
7581
const content = document.getElementById(typeGroupId + '-content');
76-
const button = document.querySelector(`[onclick="toggleTypeGroup('${typeGroupId}')"] .type-toggle`);
82+
const group = document.querySelector(`.type-group[data-type-group-id="${typeGroupId}"]`);
83+
const button = group?.querySelector('.type-toggle');
7784
const icon = button?.querySelector('.toggle-icon');
7885

79-
if (!content || !button || !icon) return;
86+
if (!content || !group || !button || !icon) return;
8087

8188
if (content.classList.contains('collapsed')) {
8289
// Expand type group
@@ -177,19 +184,12 @@ function initializeTypeGroups() {
177184
const typeGroups = document.querySelectorAll('.type-group');
178185

179186
typeGroups.forEach(typeGroup => {
180-
const typeHeader = typeGroup.querySelector('.type-header');
181187
const typeContent = typeGroup.querySelector('.type-changes');
182188
const button = typeGroup.querySelector('.type-toggle');
183189
const icon = button?.querySelector('.toggle-icon');
190+
const typeGroupId = typeGroup.getAttribute('data-type-group-id');
184191

185-
if (!typeHeader || !typeContent || !button || !icon) return;
186-
187-
// Extract type group ID from onclick attribute
188-
const onclickAttr = typeHeader.getAttribute('onclick');
189-
const match = onclickAttr?.match(/toggleTypeGroup\('([^']+)'\)/);
190-
const typeGroupId = match ? match[1] : null;
191-
192-
if (!typeGroupId) return;
192+
if (!typeContent || !button || !icon || !typeGroupId) return;
193193

194194
// Check session storage first
195195
const savedState = getTypeGroupState(typeGroupId);

0 commit comments

Comments
 (0)