-
Notifications
You must be signed in to change notification settings - Fork 52
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
fix: Remove all punctuation from anchors in policies.md #565
base: main
Are you sure you want to change the base?
Conversation
dd99b99
to
566bc25
Compare
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.
Thanks for the PR!
internal/commands/document.go
Outdated
@@ -186,7 +186,14 @@ func getDocumentation(path string, outputDirectory string) (map[rego.Severity][] | |||
} | |||
|
|||
anchor := strings.ToLower(strings.ReplaceAll(documentTitle, " ", "-")) | |||
anchor = strings.ReplaceAll(anchor, ":", "") | |||
// Tab and all ASCII punctuation except - and _ | |||
punctReplacer := strings.NewReplacer( |
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.
Can we add test coverage for this?
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 examples above test it a bit, but I guess I can add a more inclusive test
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 examples change shows it working at this point in time. The goal of test coverage is to ensure that when the code changes, this continues to work as expected. It doesn't have to be an exhaustive test, just a simple unit test that makes sure the replacers are being triggered.
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'm a little confused. Acceptance.bats ensures that the output remains correct, and there are no Go tests currently on document.go. Unless I'm missing something.
14cdf34
to
1780a51
Compare
So uh, I added a test in the form of an example. Which isn't ideal, but it's the only way to be sure that not only does the code work, the Markdown does what it says on the tin. I found a number of odd things in the course of that, including the part where cmd-click in VSCode works on the anchor in the editor (once I removed trailing space), but clicking in my Markdown preview plugin doesn't. It seems to be ok on GitHub, though, and it's a pretty contrived example to begin with. I'll do a little more poking around tomorrow just in case. |
1780a51
to
179cd06
Compare
179cd06
to
74a5edb
Compare
Ok a bunch more work later, I think this is as thorough as it can possibly be without being unreasonably complicated and resource-consuming. |
74a5edb
to
32e0fd0
Compare
32e0fd0
to
fd5598f
Compare
fd5598f
to
3f12535
Compare
Punctuation breaks anchor links in Markdown. This implements the same rule as GitHub.
(While testing, I realized that it's probably also important to escape Markdown characters in other fields. I didn't do that for the description, in case someone actually wants to use Markdown there.)