-
Notifications
You must be signed in to change notification settings - Fork 29
Implements #1552: Change how the title in the head is defined. #1832
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This currently breaks the integration tests:
(Information, , 15: 2025-09-05T08:38:25.8100000Z info ::e.d.t.f.topwatchFilter:: repo build-all :: Finished in '00:00:09.0448737')
(Information, , 16: 2025-09-05T08:38:25.8210000Z error::Program :: System.IndexOutOfRangeException: Index was outside the bounds of the array.)
(Information, , 17: 2025-09-05T08:38:25.8210000Z at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior))
(Information, , 18: 2025-09-05T08:38:25.8210000Z at System.Collections.Generic.Dictionary`2.set_Item(TKey key, TValue value))
I think a different extension point might be better (see other comment).
{ | ||
// Create a dictionary where keys are the titles | ||
// and values are files with that title | ||
var titleMap = new Dictionary<string, List<MarkdownFile>>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashSet<string>
will suffice we care about something being duplicate not necessary how many times since the diagnostics reporting will take care of that.
I think it makes more sense to implement this as IMarkdownExporter
implementations (maybe ValidationMarkdownExporter
).
That way it works for both docs-builder
and docs-assembler
. Here the duplicate file checking is per DocumentationSet
not overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a title is flagged as a duplicate, it is good to know the set of pages with the same title. Then the author can coordinate with the author(s) of the other pages for a solution. There is no harm in maintaining the full context.
var titleMap = new Dictionary<string, List<MarkdownFile>>(StringComparer.OrdinalIgnoreCase); | ||
foreach (var file in files) | ||
{ | ||
if (string.IsNullOrWhiteSpace(file.Title)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reported as an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating from comment
to request changes
No description provided.