Skip to content
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

Add a table of contents and fix disappearing underscores in subsections #2414

Closed
wants to merge 1 commit into from

Conversation

abelbraaksma
Copy link
Member

Fixes: #2411

As in the title. This adds a ToC to the Configuration.md, which is the core reference of all things configuration. I struggled a little with what level the subsections should be at, I ended up making the 2nd level, as opposed to the original 3rd level, as that aligned better with the ToC. Hope that's ok.

This also fixes the issue with underscores in titles being rendered improperly (actually: I hope it fixes them, I replaces the underscores with an escaped underscore, which is still legal MD, but hope that FSharp.Formatting correctly knows what to do with it). See also: fsprojects/FSharp.Formatting#389.

Note that, because of the way that MD auto-creates anchors of subsections and these anchors contain illegal characters for IDs, I had to add the anchors manually. I did this automated, but for new sections being added, this pattern should be repeated and the ToC should be re-updated.

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thank you for taking the time to propose this TOC.

Looking at the rendered result I'm not sure how big of an improvement this is.
image

There is a long list of items and I feel when you are looking for something specific you would ctrl + f your way to it because of the number of items in the list.

Perhaps it is better to make the search box work when you enter a setting name to get to the right location.

More info: https://marketplace.visualstudio.com/items?itemName=yzhang.markdown-all-in-one#table-of-contents
-->
## Index<!-- omit in toc -->
- [Configuration](#configuration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This anchor is wrong, should be #Configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want you to start using fsdocs here 😔.
That is the thing we use to generate the output:
image

https://github.com/fsprojects/fantomas/blob/master/docs/.README.md

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d want to, I just didn’t know how (at least not trivially). Looks like fsdocs does not read the added anchors. They were there to prevent ambiguity in the auto creation of header anchors, which apparently each markdown reader does differently.

So, the code explicitly has the lowercase c here, fsdocs just totally ignores it…

-->
## Index<!-- omit in toc -->
- [Configuration](#configuration)
- [Default `.editorconfig` settings](#default-editorconfig-settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be #Default-settings please verify all these links locally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -53,9 +104,12 @@ fsharp_strict_mode=false
Please note that you should only add settings to the `.editorconfig` file when you want to deviate from the default settings.
Copying the entire list above is unnecessary.

### indent_size
# Each setting explained
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really a fan of having multiple h1 headers on the same page.
We don't do this on the other pages, the h1 should be the title of the page and only occur once.
Please address.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nojaf, yes, that was my main point here. I didn't really like that either, but it originally rendered better with the ToC. I can remove Configuration from the ToC, but as you mentioned above, maybe there won't be a ToC at all. So either way, I'll resolve.

# Each setting explained

<a id="indent_size"></a>
## indent\_size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence about this one. Escaping work and appears to be legal Markdown, however, it is a missed opportunity to tackle fsprojects/FSharp.Formatting#389.

Copy link
Member Author

@abelbraaksma abelbraaksma Aug 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nojaf I had a quick look at that and that should be solved, sure, but I lack the time and don't know enough (yet) of that lib. Also, if I'm not going to fix it, the bug has been out there for 6 years... hence this approach for the time being.

<!--
This index can be auto-generated with VS Code's Markdown All in One extension.
The ToC will be updated-on-save, or can be generated on command by using
Ctrl-Shift-P: "Create table of contents".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a fan of using a format because of the convenience of a certain plugin in a certain editor. My editor (Rider) does something different, so this is a bit unfortunate there is no real standard.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nojaf I agree. But yeah, there's no standard. And of course, other plugins can be used. I'll remove the comment.

@abelbraaksma
Copy link
Member Author

abelbraaksma commented Aug 8, 2022

Looking at the rendered result I'm not sure how big of an improvement this is.

Is there a trivial way to see the rendered result from the repo? I just checked "View file", which showed the links working, but I have the feeling that FSharp.Formatting uses a different rendering of certain anchors. The simple solution would be to just add specific anchors to each title.

Perhaps it is better to make the search box work when you enter a setting name to get to the right location.

@nojaf That could work. Or a collapsible ToC. Not sure there's any (standard?) way of doing either, though, as it should be on the page. I do have to say, while working on it, it was nice to be able to click back and forth between the items. The page is rather long, and the ToC is a little bit more readable as a list of available options than the defaultConfig section.

@LyndonGingerich
Copy link

LyndonGingerich commented Aug 8, 2022

Perhaps I'm missing something, but won't fsprojects/FSharp.Formatting#760 render this PR at least partially redundant?

@abelbraaksma
Copy link
Member Author

Yes, and this PR was the trigger, apparently, for @nojaf to fix it. Which is great!

@abelbraaksma
Copy link
Member Author

@nojaf, thanks for that fix, and glad it got merged so quickly! I guess I can undo most changes (glad so, it’s better with proper support!). Would still like to have the ToC in, but if you think it’s to intrusive, we can probably close this with no action.

@nojaf
Copy link
Contributor

nojaf commented Aug 16, 2022

Hello @abelbraaksma, following up on this, we would go for a slightly different approach in #2427.
Thanks again for triggering progress with this PR.

@nojaf nojaf closed this Aug 16, 2022
@abelbraaksma
Copy link
Member Author

@nojaf no worries, glad I triggered the discussion:)

@abelbraaksma abelbraaksma deleted the add-toc branch August 16, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Underscores in subsections of configuration docs disappear, plus suggestion for table of contents
3 participants