-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Add minimal Vale configuration, existing package files #220
base: main
Are you sure you want to change the base?
Conversation
This commit adds a new minimal Vale configuration, as well as some package files from a prior iteration of a style guide for NGINX/Vale. There is a lot of nuance to how Vale can be configured: the intent of adding these files is so that we can begin to iterate on what to override, add or disable alongside style guide work.
✅ Deploy Preview will be available once build job completes!
|
I'm testing Vale against some NGINX Gateway Fabric pages while I figure out what the good, contemporary way to configure Vale is. Although anyone reading this PR for the first time has no context about it, the old Vale configuration file had some precise overrides for Microsoft styles that we didn't always have a cascade for, and there were a lot of precise patterns for ignores in Markdown files that are now managed by the Hugo package. |
|
||
[*.{md}] | ||
|
||
BasedOnStyles = Vale, Microsoft, NGINX |
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 see you've included NGINX Acronyms.yml
Partially as a test, I set this up to avoid the rules associated with https://github.com/errata-ai/Microsoft/blob/master/Microsoft/Acronyms.yml
BasedOnStyles = Vale, Microsoft, NGINX | |
BasedOnStyles = Vale, Microsoft, NGINX | |
# Style.Rule = {YES, NO, suggestion, warning, error} to | |
# enable/disable a rule or change its level. | |
Microsoft.Acronyms = NO |
With this modification, I avoided this error message:
$ vale sync
$ vale content/nginx-one/getting-started.md
content/nginx-one/getting-started.md
10:89 suggestion 'NGINX' has no definition. Microsoft.Acronyms
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 don't see the value in re-documenting how Vale works.
I am going through active testing and adjustment of the rules: this PR is in a draft state while I do so, and as such, I am not accepting edit suggestions.
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 suggesting that we follow good coding practice. IOW, add comments that explain what's happening with the code.
this PR is in a draft state while I do so, and as such, I am not accepting edit suggestions.
Now you know what I'm looking for once you're ready to take this PR out of draft state. And even more, with this comment, I can remember what I think is needed when this PR is ready for review.
@@ -0,0 +1,8 @@ | |||
StylesPath = styles |
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 want it clear that this is a PoC for now....
StylesPath = styles | |
# This is a PoC, until we have a configuration that matches our style guide at | |
# https://github.com/nginx/documentation/blob/main/templates/style-guide.md | |
StylesPath = styles |
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.
Does the PR being in a draft state not create sufficient enough barrier to entry?
Someone would have to check out the branch to attempt to use it.
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 guess I don't understand your approach to this PR / change. Once it's taken out of draft, reviewed, and merged, I'm thinking this will be a PoC for a while -- until we're able to test this on a number of NGINX docs.
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 don't intend to swap this to a review state until it's been tested across multiple documentation sets, and even then, it's an optional artifact for people to interact with until it's enforced as a requirement by both consensus and CI tooling.
Proposed changes
This pull request adds a new minimal Vale configuration, as well as some package files from a prior iteration of a style guide for NGINX/F5. There is a lot of nuance to how Vale can be configured: the intent of adding these files is so that we can begin to iterate on what to override, add or disable alongside style guide work.
Checklist
Before merging a pull request, run through this checklist and mark each as complete.
README.md
andCHANGELOG.md
Footnotes
Potentially sensitive changes include anything involving code, personally identify information (PII), live URLs or significant amounts of new or revised documentation. Please refer to our style guide for guidance about placeholder content. ↩