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

[Docs] Fix warnings and suggestions from vale #540

Merged
merged 7 commits into from
Jun 21, 2024
Merged

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Jun 21, 2024

Description

  • Fix all warnings and suggestion from vale that were not visible until an error was introduced

@stichbury - returning to your comment on why certain things were not raised on Vale in the last PRs. I just quickly checked, and there are two relevant factors:

  • Previously, e.g. was considered an error, but e.g was not -> I've made it stricter in this PR now
  • It seems like vale as a pre-commit hook will always return "passed" as long as there is no new error introduced (3 different levels: error, warning, suggestion).

This means that over time, vale probably raised a couple of suggestions and warnings, but they were never shown to us unless we simultaneously introduced a vale error. To find all of the warnings/suggestions, I intentionally added an error (adding e.g. this in any of the docs) and then ran the linter. Only then were all of the warnings/suggestions visible. I've fixed them all in this PR now, but it's essential to be aware of that limitation.

@stichbury - I couldn't find a quick way of configuring that warnings/suggestions are also errors for us. Do you have an idea? Otherwise, it's good to be aware of the restriction above 🔝 It might be worth considering switching to github actions for vale. It seems like there is a configuration option fail_on_error that might does that, but one would have to test it out, because I am not sure if it behaves similarly to the pre-commit hook. So I'll leave that for someone to test out next sprint 👍

Screenshot

Notice

  • I acknowledge and agree that by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

@huong-li-nguyen huong-li-nguyen self-assigned this Jun 21, 2024
@huong-li-nguyen huong-li-nguyen changed the title [Docs] Fix warnings and suggestion from vale [Docs] Fix warnings and suggestions from vale Jun 21, 2024
@huong-li-nguyen huong-li-nguyen added Docs 🗒️ Issue for markdown and API documentation Status: Ready for Review ☑️ labels Jun 21, 2024
@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review June 21, 2024 11:12
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

🧹 😍

.vale/styles/Microsoft/Foreign.yml Outdated Show resolved Hide resolved
@stichbury
Copy link
Contributor

Aha! That makes sense, thanks for working out what was happening and resolving those issues.

I couldn't find a quick way of configuring that warnings/suggestions are also errors for us. Do you have an idea?

We could just make the level setting of the Vale styles that are important into errors rather than warnings/suggestions? It's not non-trivial but it's relatively easy to do for the main culprits that we occur regularly such as spelling, capitalisation and abbreviation.

Of course, by doing that, it then means that all reports have to be fixed even where they are valid cases (this may mean simply switching vale on and off with comments around the section in question).

Having a way to see all suggestions clearly at all times, and then decide whether to act on them is the ideal. I'll discuss with @antonymilne in the next sprint as we have a ticket to look at this. For now though, you've cleared up what was there, and that's awesome. Thank you 🙏

Copy link
Contributor

@stichbury stichbury left a comment

Choose a reason for hiding this comment

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

Nice modifications to make Vale happy, thank you! 🏆

@huong-li-nguyen huong-li-nguyen merged commit 627d5c8 into main Jun 21, 2024
40 checks passed
@huong-li-nguyen huong-li-nguyen deleted the tidy/fix-vale branch June 21, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants