Add initial vale configuration and apply editorial changes to documentation #3567
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
added
🟨 priority: medium
|
Size Change: +2 B (0%) Total Size: 1.06 MB ℹ️ View Unchanged
|
|
An alternative to using a published image is to run the vale linting step using |
sarayourfriend
force-pushed
the
fix/address-vale-errors
branch
from
ab996cc to
80e5b21
Compare
krysal
reviewed
Jan 5, 2024
|
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @stacimc Excluding weekend1 days, this PR was ready for review 11 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
stacimc
approved these changes
Jan 20, 2024
There was a problem hiding this comment.
Very Exceptionally cool :) Documentation improvements all look good to me as well; this is a welcome change!
I agree with you re: project proposals and especially the changelogs.
One thing I noticed when testing is that Vale appears to ignore anything in code blocks using the backticks. In our documentation some of these are actual code blocks but others are notes/warnings that probably ought to be checked. For example if I add this in a doc file, it won't raise any warnings or errors:
```{warning}
This note is very likely to to cause Vale errors.
```
Not a blocker but perhaps something to investigate in a follow up? I looked briefly and was unable to determine if this is a bug or intentional behavior by Vale.
| @@ -7,7 +7,7 @@ | |||
| https://api.inaturalist.org/v1/docs/ | |||
| But there is a full dump intended for sharing on S3. | |||
| https://github.com/inaturalist/inaturalist-open-data/tree/documentation/Metadata | |||
| Because these are very large normalized tables, as opposed to more document | |||
| Because these are exceptionally large normalized tables, as opposed to more document | |||
There was a problem hiding this comment.
I initially left a comment confused why this was updated, because I was not able to reproduce any Vale errors by modifying any files outside of documentation locally (and didn't expect to be able to, either). I realized it's because this docstring is added to DAGs.md via generate-dag-docs, of course! Leaving the comment anyway in case anyone else has the same confusion 😅
There was a problem hiding this comment.
Yes, that is precisely why. Sorry, I should have left a note regarding this particular change.
krysal
approved these changes
Jan 22, 2024
|
|
||
| # Run Vale configured for Openverse in Docker. | ||
| # Using Docker avoids the need for contributors to install the Vale binary. | ||
| #Configuration defaults to what is used for CI. |
There was a problem hiding this comment.
Suggested change
| #Configuration defaults to what is used for CI. | |
| # Configuration defaults to what is used for CI. |
I think this is intentional, for the same reason that prettier ignores formatting in code blocks (#2348). I think we can actually get Vale to stop ignoring those codeblocks by changing the default setting for I'll try that and see if it creates any false positives with actual code blocks. Edit: Actually we also need to change the "IgnoredScopes" setting. Need to remove This does result in false positives: We can change The "very" is a test I put in using your example, Staci. I'll make those changes and see if I can get it working without false positives, without being tedious. |
sarayourfriend
force-pushed
the
fix/address-vale-errors
branch
from
143aa7b to
db878f7
Compare
|
Okay, I've updated the PR with the following:
|
sarayourfriend
force-pushed
the
fix/address-vale-errors
branch
from
db878f7 to
3190d7b
Compare
dhruvkb
approved these changes
Jan 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #3566 by @dhruvkb
Description
I've picked this work up from @ngken0995 so that Ken can focus on other PRs.
When we originally started looking into Vale, we were misinformed about how it worked. We thought that you had to commit the styles files to the repository. However, that's not the intended way of using Vale. Instead, the Vale documentation instructs you to git-ignore all "synced" configuration files, and only commit custom configuration files. This allows us to avoid introducing big long lists of offensive terms into the monorepo. Those files would still appear if you ran
vale syncfrom within the new.valedirectory. But that is not necessary to do, not even when developing the Vale configuration, because everything can run from within the built docker image. Running Vale using Docker also allows us to avoid contributors needing to install Vale locally, and avoids potential conflicts in Vale versions by pinning the Vale version we use in the base docker image's tag.We will not use a published image in any circumstance because: (a) the vale docker image is small and after the first time it is downloaded it is reused every time, unlike needing to redownload the entire published image each time the configuration changes, and (b) it avoids a whole host of complexity in our CI and local linting when changes to the Vale configuration conflict with the previous Vale configuration. For example, if a new configuration conflicts with the currently published configuration, the Vale pre-commit step would fail when running the published image! This was too hard to resolve, and would require looking at the git history compared to
HEADinpre-commit, or juggling new conditionals in our already complex ci_cd.yml workflow. Using a local image every time avoids all of this complexity without any downsides. In fact, it is positive because the base vale image is always reused instead of needing to redownload it any time the Vale configuration changes (excluding updates to the version of the base image).Run vale using
just .vale/run. This also builds the docker image. Refer to.vale/README.mdand other new in-code documentation for rationale behind various configuration choices.The pre-commit hook also excludes project proposals and changelogs. Changelogs are automatically generated, and as such, I don't think they should be linted for editorial content. For example, if a PR title doesn't pass Vale, I don't think it's accurate to change the title in the changelog. That's my personal judgement, but if other folks want to run Vale there, I would also think that's fine and wouldn't push against it.
The project proposals, on the other hand, are complex. They raise a lot of errors in Vale and are the bulk of our documentation, line-by-line. They are also not "living" documents, that we make changes to over time. Once they're in, they're more or less meant to stay as-is, with changes to the project reflected in the project threads (that's my understanding). Going back and making retroactive editorial changes to those files doesn't feel right to me. On the other hand, if folks do want to do that, then I also wouldn't push back here, I'd just ask that it gets done in a separate PR.
This configuration is just a starting point, and we can incorporate additional rules in the future, if we like, even from other style guides. Red Hat's guide in particular has a lot of compelling options, including scentence complexity checks and a much better passive voice check than the other ones I've seen.
Testing Instructions
Run
just .vale/run. Make changes that will cause issues with Vale and confirm they raise errors as you expect. Confirm no unnecessary errors are present.Read over the Vale configuration and through the Vale documentation to confirm that it all makes sense and is appropriate for our project.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin