Add sort and exclude options for folder auto-discovery in toc.yml#2870
Add sort and exclude options for folder auto-discovery in toc.yml#2870
Conversation
🔍 Preview links for changed docs |
|
This generally LGTM. Thank you for the contribution! Is this enough for sorting versions? I expect unintended behaviour when we go past e.g. but if we sort this ascending or descending. It will become ascending: or descending: |
|
@barkbay if you intend to use this for ECK, it should be paired with some sort of exclusion logic so we can hide |
Good catch 🤦 I updated the PR with 10f4ab3 to use natural sort order (comparing numeric segments as integers) when To be conservative, natural sort is only used when I also added |
@shainaraskas good point! Should I create a new PR? I'm asking as it feels like a separate concern from sort order. Happy to open a follow-up PR. |
10f4ab3 to
eeb12c8
Compare
eeb12c8 to
db15588
Compare
Maybe not a big change, so it I decided to implement in a20edc2 Happy to also create a dedicated PR. |
|
I'll let dev opine on whether |
There was a problem hiding this comment.
Pull request overview
This PR extends toc.yml folder auto-discovery to support configurable file ordering (sort) and file filtering (exclude) when children are not explicitly defined, including natural sorting for version-like filenames.
Changes:
- Parse and carry
sort/excludefromtoc.ymlintoFolderRef, validatesort, and apply sorting + filtering during folder auto-discovery. - Introduce
SortOrder, parsing helpers, and aNaturalStringComparerto support natural ordering whensortis explicitly set. - Add/expand tests and update navigation documentation to describe the new options.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Navigation.Tests/Isolation/NaturalStringComparerTests.cs | Adds tests for natural string comparison behavior (currently contains compile errors). |
| tests/Navigation.Tests/Isolation/FolderSortOrderTests.cs | Adds tests for sort behavior in folder auto-discovery. |
| tests/Navigation.Tests/Isolation/FolderExcludeTests.cs | Adds tests for exclude behavior in folder auto-discovery. |
| src/Elastic.Documentation.Configuration/Toc/TableOfContentsYamlConverters.cs | Extends YAML converter to deserialize sort and exclude for folder items. |
| src/Elastic.Documentation.Configuration/Toc/TableOfContentsItems.cs | Adds SortOrder, parsing extensions, NaturalStringComparer, and extends FolderRef. |
| src/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cs | Applies sort/exclude during folder auto-discovery and emits diagnostics for invalid sort. |
| docs/configure/content-set/navigation.md | Documents the new sort and exclude options for folder auto-discovery. |
| var same = "abc"; | ||
| Comparer.Compare(same, new string(same)).Should().Be(0); | ||
| } |
There was a problem hiding this comment.
new string(same) does not compile because string has no constructor that takes a string. If you need a different instance with the same content (to exercise the non-ReferenceEquals path), create it via same.ToCharArray() (or similar) instead.
There was a problem hiding this comment.
I'm not sure I understand this. According to Claude string has an implicit conversion to ReadOnlySpan<char>, and there is a string(ReadOnlySpan<char>) constructor.
When compiling new string(same) the compiler sees:
- No string(string) constructor — doesn't match
- same implicitly converts to ReadOnlySpan — matches string(ReadOnlySpan) — compiles
This code works perfectly fine with .NET 10
There was a problem hiding this comment.
@copilot new string(string) compiles fine on .NET Core 2.1+ because string implicitly converts to ReadOnlySpan, which matches the string(ReadOnlySpan) constructor. The tests pass. Can you confirm?
| var a = "test"; | ||
| var b = new string(a); | ||
| Comparer.Compare(a, b).Should().Be(0); |
There was a problem hiding this comment.
new string(a) does not compile because there is no string constructor that accepts a string. If the goal is to compare equal values with different references, construct from a char[] or another explicit copy mechanism.
| var start = index; | ||
| while (index < s.Length && char.IsDigit(s[index])) | ||
| index++; | ||
| return long.Parse(s[start..index], CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
ParseNumber uses long.Parse(...) which can throw OverflowException if a filename contains a numeric segment larger than Int64.MaxValue (and would fail the docs build when sort is set). Consider using long.TryParse with a deterministic fallback comparison (e.g., compare digit-run lengths, then ordinal compare) to keep sorting total and non-throwing.
| 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.
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.
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?
| // 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); | ||
| return new FolderRef(fullPath, pathRelativeToContainer, resolvedChildren, context, folderRef.Sort); |
There was a problem hiding this comment.
When resolving a FolderRef that has explicit children, the returned FolderRef drops the Exclude value (new FolderRef(..., folderRef.Sort)), so the resolved model no longer reflects the original YAML configuration. Even if exclude is ignored for explicit children, consider passing through folderRef.Exclude for consistency/debuggability.
| 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.
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
src/Elastic.Documentation.Configuration/Toc/DocumentationSetFile.cs
Outdated
Show resolved
Hide resolved
| var same = "v1"; | ||
| Comparer.Compare(same, new string(same)).Should().Be(0); | ||
| } |
There was a problem hiding this comment.
new string(same) does not compile (no string ctor overload taking string). Use a safe way to create a distinct string instance (e.g., from same.ToCharArray()) or remove the redundant allocation if reference identity isn't important here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
You can also share your feedback on Copilot code review. Take the survey.
This is my very first code change in C# and in this project, so bear with me! Code was generated with the help of an AI agent but I tried to review and understand it with my 🧠 .
This PR adds two options to folder entries in
toc.ymlfor auto-discovered files:sort— control file orderingPlease refer to this comment for the original need.
Valid values:
asc,ascending,desc,descending. Defaults toascending.sortis specified, files are sorted alphabetically (A-Z), preserving existing behavior.sortis set, natural sort order is used so version numbers sort correctly (3_2_0comes before3_10_0).index.mdis always placed first regardless of sort order.childrenare defined, sort has no effect — the user-defined order is respected.exclude— filter out specific fileschildrenare explicitly defined).Test plan
sortis omittedsort: desc/sort: descendingorders files Z-Asort: asc/sort: ascendingorders files A-Zindex.mdalways placed first even with descending sortfolder+filecombo preserves sort valuechildrenignore sort orderindex.mdwhen explicitly listed