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 escaping for links and headers in docsgen #2330

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ilyakuz-db
Copy link
Contributor

Changes

To avoid build warnings and errors in docs build we need to escape symbols that are treated as syntax elements

Tests

func getDescription(s *jsonschema.Schema, allowMarkdown bool) string {
if allowMarkdown && s.MarkdownDescription != "" {
func getDescription(s *jsonschema.Schema) string {
if s.MarkdownDescription != "" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was an old workaround which is not needed anymore

@@ -414,16 +414,6 @@ github.com/databricks/cli/bundle/config/resources.Permission:
"user_name":
"description": |-
The name of the user that has the permission set in level.
github.com/databricks/cli/bundle/config/resources.Pipeline:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These fields are already defined in annotations_openapi_overrides.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we test that we don't have duplicate entries?

s = strings.ReplaceAll(s, "<", "\\<")
s = strings.ReplaceAll(s, ">", "\\>")
return s
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include a few tests with cases that you need to address as examples?

The replacements look quite broad. Are these characters categorically rejected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The replacements look quite broad. Are these characters categorically rejected?

URL validation is failing because the framework is trying to treat these characters as HTML tag brackets. In runtime it seemed to work when I tested

New docs framework seems to enforce this, so it doesn't work even at the build level

@@ -43,7 +43,7 @@ artifacts:

* - `files`
- Sequence
- The source files for the artifact. See [_](#artifacts.<name>.files).
- The source files for the artifact. See [_](#artifactsnamefiles).
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the docs backend produce these anchor names?

If so it would be good to call this out in the function that strips the characters. And as a side note, are there additional characters that it strips if this is indeed the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the docs backend produce these anchor names?

Yes, it is produced like this in resulting HTML by docs framework (it's not exactly backend but build-tool), I'll add a note

And as a side note, are there additional characters that it strips if this is indeed the case?

Probably but we don't expect any weird symbols in our links/titles because all of these are field names from config. The only exceptions are these fancy brackets that we add for <name>

We have URL validation in docs CI that catch any mismatch so I wouldn't strip anything until necessary, at least until framework migration is finished

@@ -414,16 +414,6 @@ github.com/databricks/cli/bundle/config/resources.Permission:
"user_name":
"description": |-
The name of the user that has the permission set in level.
github.com/databricks/cli/bundle/config/resources.Pipeline:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we test that we don't have duplicate entries?

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.

2 participants