I think the test is now poorly named, since it tests more than CustomIcons at this point. In my mind there are two alternatives. Either we break off the tests into individual tests and instead of XCTAssertEqual on the full string, we assert on the presence of individual elements. alternatively we just rename this test to testRenderIndexGenerationWithBookLikeContent although I am not sure I like to refer to all of these as being book-like since I can see things like this new tab navigator being useful for large amounts of related screenshots in an article within conceptual documentation for an app or something.

Ah I disagree. I think it's still only testing custom icons- if the navigator index doesn't include just regular article pages in it then something has gone really wrong.

I just had to update the test to reflect the content of the DocC catalog since I added an additional page for a different test in RenderNodeTranslatorTests but the main thing this is testing for is still the special page that customizes an icon.

I see what you mean, I think it's fine for this review then. FTR though I think that for someone new to the code base without the context of this PR, it is less clear what the test is checking for. We should come up with a better way of testing individual render index features without having to do this all the time.

Not sure this kind of test belongs in here. It's more checking the directive parser than the tab navigator directive itself. I think as you already went through so much effort to make directive arguments to content so declarative IMHO there is no point checking random keys like that.

Hm yeah that's fair. I'm asserting on the generated RenderBlockContent when an unknown directive is encountered as well below which is a little more interesting but I agree that bestTab: true is kind of useless at this point. I can remove it.

The unknown directive case is more interesting in the sense that it maps more closely to the structure we expect from the content specifically for tab navigators.

The double sort feels really confusing. there must be a better way of achieving this. If we just need them sorted by line number then let's just keep the second sort. If we then need ordering on other criteria let's make it explicit and do a simple, .sorted.

Ah I'm open to suggestions. We need it sorted on the line number so that the tests are readable but then we need the sort to be stable between test runs for multiple diagnostics that appear on the same line so we need to sort on the full string as well.

I considered dropping into a full sorted but I think those are inherently much more confusing to read.

Right now you are relying on the sorting algorithm being stable. I think that returning a tuple of (line: Int, id: String) and just doing sorted is clearer. I really think that intuitively people know that tuples are sorted element wise from "left to right". Alternatively, if readability is a concern here consider making the initial map return something very structure like (line: Int, severity: WhateverTypeThisIs, diagID: WhateverTypeThisIs) or whatever else makes sense and then making the final map in the expression something like:

.map { "\($0.line): \($0.severity) - \($0.id)" }

this has the benefit that it separates figuring out the order and generating the strings, so that people can focus the logical chunks independently.

Oh I just hadn't considered sorting the tuple itself – will that work? Agreed that's a much nicer solution if so. (I'll try it.)

Hm yeah a (Int, String) isn't comparable so just sorted() doesn't work unfortunately. I'll implement it manually and see how it is.

Okay I moved to a custom .sorted. Let me know what you think.

Interestingly enough you could write the sorted as .sorted { $0 < $1 } because tuples somehow implement <` without conforming to comparable apparently. I still think your version is better though