-
Notifications
You must be signed in to change notification settings - Fork 38
Add sort and exclude options for folder auto-discovery in toc.yml #2870
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?
Changes from 12 commits
8ffc25a
ecdd232
a982e1e
db15588
a20edc2
d2ef58f
80fae41
3c1cdd4
667954b
dcd7be8
f9a58ce
da9aafa
5996a20
1234cef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -478,31 +478,44 @@ private static ITableOfContentsItem ResolveFolderRef(IDiagnosticsCollector colle | |||||
| ? fullPath | ||||||
| : fullPath.Substring(containerPath.Length + 1); | ||||||
|
|
||||||
| // Parse and validate sort order | ||||||
| if (!SortOrderExtensions.TryParse(folderRef.Sort, out var sortOrder) && folderRef.Sort is not null) | ||||||
| collector.EmitError( | ||||||
| context, | ||||||
| $"Unknown sort order '{folderRef.Sort}' for folder '{folderRef.PathRelativeToDocumentationSet}'." | ||||||
| + " Valid values are: asc, ascending, desc, descending." | ||||||
| ); | ||||||
|
|
||||||
| // If children are explicitly defined, resolve them | ||||||
| if (folderRef.Children.Count > 0) | ||||||
| { | ||||||
| // For children of folders, the container remains the same as the folder's container | ||||||
| var resolvedChildren = ResolveTableOfContents(collector, folderRef.Children, baseDirectory, fileSystem, fullPath, containerPath, context, suppressDiagnostics); | ||||||
| return new FolderRef(fullPath, pathRelativeToContainer, resolvedChildren, context); | ||||||
| // Exclude is intentionally not passed through — it only applies to auto-discovery | ||||||
| return new FolderRef(fullPath, pathRelativeToContainer, resolvedChildren, context, folderRef.Sort); | ||||||
|
||||||
| return new FolderRef(fullPath, pathRelativeToContainer, resolvedChildren, context, folderRef.Sort); | |
| return new FolderRef(fullPath, pathRelativeToContainer, resolvedChildren, context, folderRef.Sort, folderRef.Exclude); |
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 is intentional, exclude only applies to auto-discovery. Carrying it through when children are explicit would suggest it has an effect when it doesn't. Tried to clarify this in 80fae41
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,8 +2,94 @@ | |||||||||||||||||||
| // Elasticsearch B.V licenses this file to you under the Apache 2.0 License. | ||||||||||||||||||||
| // See the LICENSE file in the project root for more information | ||||||||||||||||||||
|
|
||||||||||||||||||||
| using System.Globalization; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| namespace Elastic.Documentation.Configuration.Toc; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Specifies the sort order for auto-discovered files in a folder. | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
| public enum SortOrder | ||||||||||||||||||||
| { | ||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Sort files in ascending alphabetical order (A-Z). This is the default. | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
| Ascending, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary> | ||||||||||||||||||||
| /// Sort files in descending alphabetical order (Z-A). | ||||||||||||||||||||
| /// Useful for version-numbered folders where newest should appear first. | ||||||||||||||||||||
| /// </summary> | ||||||||||||||||||||
| Descending | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary>Parsing helpers for <see cref="SortOrder"/>.</summary> | ||||||||||||||||||||
| public static class SortOrderExtensions | ||||||||||||||||||||
| { | ||||||||||||||||||||
| /// <summary>Tries to parse a YAML sort value (asc, ascending, desc, descending) into a <see cref="SortOrder"/>.</summary> | ||||||||||||||||||||
| public static bool TryParse(string? value, out SortOrder result) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| var normalized = value?.ToLowerInvariant(); | ||||||||||||||||||||
| (result, var valid) = normalized switch | ||||||||||||||||||||
| { | ||||||||||||||||||||
| "desc" or "descending" => (SortOrder.Descending, true), | ||||||||||||||||||||
| "asc" or "ascending" => (SortOrder.Ascending, true), | ||||||||||||||||||||
| _ => (SortOrder.Ascending, false) | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| return valid; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /// <summary>Compares strings using natural sort order, where numeric segments are compared as integers ("3_2" < "3_10").</summary> | ||||||||||||||||||||
| public sealed class NaturalStringComparer : IComparer<string> | ||||||||||||||||||||
| { | ||||||||||||||||||||
| public static NaturalStringComparer Instance { get; } = new(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| public int Compare(string? x, string? y) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (ReferenceEquals(x, y)) | ||||||||||||||||||||
| return 0; | ||||||||||||||||||||
| if (x is null) | ||||||||||||||||||||
| return -1; | ||||||||||||||||||||
| if (y is null) | ||||||||||||||||||||
| return 1; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| var ix = 0; | ||||||||||||||||||||
| var iy = 0; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| while (ix < x.Length && iy < y.Length) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| if (char.IsDigit(x[ix]) && char.IsDigit(y[iy])) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| // Compare numeric segments as integers | ||||||||||||||||||||
| var nx = ParseNumber(x, ref ix); | ||||||||||||||||||||
| var ny = ParseNumber(y, ref iy); | ||||||||||||||||||||
| var cmp = nx.CompareTo(ny); | ||||||||||||||||||||
| if (cmp != 0) | ||||||||||||||||||||
| return cmp; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| else | ||||||||||||||||||||
| { | ||||||||||||||||||||
| var cmp = x[ix].CompareTo(y[iy]); | ||||||||||||||||||||
| if (cmp != 0) | ||||||||||||||||||||
| return cmp; | ||||||||||||||||||||
| ix++; | ||||||||||||||||||||
| iy++; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return x.Length.CompareTo(y.Length); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| private static long ParseNumber(string s, ref int index) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| var start = index; | ||||||||||||||||||||
| while (index < s.Length && char.IsDigit(s[index])) | ||||||||||||||||||||
| index++; | ||||||||||||||||||||
| return long.Parse(s[start..index], CultureInfo.InvariantCulture); | ||||||||||||||||||||
|
||||||||||||||||||||
| return long.Parse(s[start..index], CultureInfo.InvariantCulture); | |
| var segment = s.Substring(start, index - start); | |
| if (long.TryParse(segment, NumberStyles.None, CultureInfo.InvariantCulture, out var value)) | |
| return value; | |
| // Fallback for values that do not fit in Int64 (e.g., very long digit runs). | |
| // Returning a fixed sentinel keeps comparisons deterministic and non-throwing. | |
| return long.MaxValue; |
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.
Valid point. A filename with a huge numeric segment would throw. However, this is extremely unlikely for documentation file names. If we want to be defensive, the long.TryParse fallback is reasonable. But the suggested fallback of returning long.MaxValue would make all oversized numbers compare equal, which isn't great 🤷 A better fallback might be to compare by digit-run length first (longer = larger), then lexicographically.
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.
Digging into this, the overflow would only matter for filenames with numeric segments larger than 9,223,372,036,854,775,807 (19+ digits). Do we think this is realistic for documentation file names?
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.
Nit: wouldn't it be better to use the SortOrder enum here with a minor tweak? From what I understood there are three possible states - correct me if I'm wrong: AscendingNatural, DescendingAlphabetical, AscendingAlphabetical.
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.
Yes, there are indeed three states:
- alphabetical (default, no sort set)
- ascending natural
- descending natural
We need the raw string on FolderRef, because the YAML converter doesn't have access to the diagnostics collector, validation happens during resolution, where we need the original value to report meaningful errors like Unknown sort order 'newest' I think using an enum here would mean losing that information or silently discarding invalid values?
Uh oh!
There was an error while loading. Please reload this page.