-
Notifications
You must be signed in to change notification settings - Fork 12
BREAKING CHANGE: Don't allow dots (.) in slug #1093
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
Conversation
Label error. Requires exactly 1 of: automation, breaking, bug, changelog:skip, chore, ci, dependencies, documentation, enhancement, feature, fix, redesign. Found: |
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.
+1 on removing dots.
var slug = slugTarget.Slugify(); | ||
var slug = anchor ?? header.Slugify(); | ||
if (slug.Contains('$')) | ||
slug = HeadingAnchorParser.InlineAnchors().Replace(slug, ""); |
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.
I do think we want to always sanitize anchor
or header
through Slugify()
.
What was the usecase for removing it from anchor
?
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.
Sorry, currently working on this. Switched back to draft.
But I was thinking of limiting the blast radius.
If someone creates a custom anchor, we might want to maintain the dot.
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.
I think having uniformity in our slugging beats allowing dots in custom anchors.
The main reason might be version numbers but they look okay without dots too e.g
#whats-new-in-8.18
vs
#whats-new-in-8-18
I actually prefer the latter visually too.
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.
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.
ahh but that discrepancy exists today already no?
If someone writes
## Whats new in 8.18
We slug to: whats-new-in-8-18
today?
## Whats new in 8.18 [whats-new-in-8.18]
I do think that should also be sanitized the same way to whats-new-in-8-18
.
We shouldn't take anchors verbatim and having two sanitation rules is a bit suprising to writers IMO.
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.
- The numbering gets removed (maybe this is intended?)
- As a consequence, the anchor is generated differently which breaks the navigation functionality (clicking on the navbar link does nothing)
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.
💩
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.
One problem I just thought of is the authoring experience.
It will be confusing if you set a custom anchor and then it's sanitized.
E.g.
If I add the custom anchor #whats-new-in-8.0
I would expect it to stay like that.
It could be confusing to see that it changed to #whats-new-in-8-0
.
Maybe the best way is to actually to discourage dots (.
) in anchors and make it 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.
We need to sanitize custom anchors anyhoo e.g #What's-new-in-8.0
we can not take it verbatim. The sanitization is not surprising to me but thats subjective :)
We could hint
if custom anchors end up different after sanitization?
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.
Won't do. The assembler build emits thousands of errors from both normal content and generated content. Hence, it's not feasible to continue this. |
Context
Currently, dots (
.
) are allowed in slugs. This means generated anchors also contain slugs.If you create a header like
## 1. Install
Then it's anchor will be
#1.-install
.However, IDE's are expecting the anchor
#1-install
.Reported by @flobernd
Changes
By removing the
.
from the allowed chars of Slugify, the correct anchor is generated.