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

Added Vale to site checks #4231

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

Conversation

atsansone
Copy link
Contributor

No description provided.

@MaryaBelanger
Copy link
Contributor

To run vale:

  1. brew install vale
  2. vale sync
  3. vale path/to/file.md

Tested, runs fine.

$ vale src/community/who-uses-dart.md

 src/community/who-uses-dart.md
 1:1    warning     Try to keep the Flesch–Kincaid  Readability.FleschKincaid        
                    grade level (9.44) below 8.                                      
 1:1    warning     Try to keep the LIX score       Readability.LIX                  
                    (45.22) below 35.                                                
 1:1    warning     Try to keep the Automated       Readability.AutomatedReadability 
                    Readability Index (9.93) below                                   
                    8.                                                               
 1:1    warning     Try to keep the Coleman–Liau    Readability.ColemanLiau          
                    Index grade (10.82) below 19.                                    
 1:1    warning     Try to keep the SMOG grade      Readability.SMOG                 
                    (10.86) below 10.                                                
 1:1    warning     Try to keep the Flesch reading  Readability.FleschReadingEase    
                    ease score (58.27) above 70.                                     
 1:1    warning     Try to keep the Gunning-Fog     Readability.GunningFog           
                    index (10.78) below 10.                                          
 14:37  warning     'many' is a weasel word!        write-good.Weasel                
 15:21  suggestion  Try to avoid using 'are'.       write-good.E-Prime               
 17:1   suggestion  Try to avoid using 'you're'.    write-good.E-Prime               
 25:2   suggestion  Spell out 'IDEAS', if it's      Google.Acronyms                  
                    unfamiliar to the audience.                                      
 25:8   suggestion  Spell out 'FOR', if it's        Google.Acronyms                  
                    unfamiliar to the audience.                                      
 26:4   error       Did you really mean             Vale.Spelling                    
                    'endcomment'?                                                    

✖ 1 error, 8 warnings and 4 suggestions in 1 file.

A couple things I would like to see:

  1. I wish the warnings about various readability grades contained links to the definition / pointers about how to improve that in the CL output. If you find the file, they do provide a link to more info on the grade there.
  2. "weasel word" would also be a nice thing to link and define. What does that mean? The write-good/Weasel file does not contain a definition (can imagine it would not be self explanatory for ESL users).
  3. Over time, we should be customizing this a lot to our style requirements, like removing things that are not vital. Excessive notifications will be a deterrent to actually implementing what Vale suggests.

Concern:

  1. The above example shows me 1 error / 8 warnings / 4 suggestions, but the VS Code integration shows be different warnings. This is the case with all the other pages I tested too. Probably just a config issue on my end, but should keep an eye out for that.

Copy link
Contributor

@MaryaBelanger MaryaBelanger left a comment

Choose a reason for hiding this comment

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

Hard to test everything, but this PR isn't forcing anyone to use it and we can improve it/pare down through our own regular use, so lgtm with that in mind

Copy link
Contributor

@MaryaBelanger MaryaBelanger left a comment

Choose a reason for hiding this comment

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

Decided to actually fit it to the google style guide before merging

Copy link
Collaborator

@sfshaza2 sfshaza2 left a comment

Choose a reason for hiding this comment

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

Woah! @atsansone, before merging a new tool like this, we need to see it staged, try it out, and make a team decision. Have you staged this?

@sfshaza2
Copy link
Collaborator

On further thought, I'm closing this PR for now. We need a lot more research before adding a tool like this. We should probably get @khanhnwin involved if we feel it's important enough. (To be honest, given my current understanding, I don't feel like I need this. Hence the research and testing.)

@sfshaza2 sfshaza2 closed this Sep 20, 2022
@atsansone atsansone reopened this May 22, 2023
@dart-github-bot
Copy link
Collaborator

dart-github-bot commented Jan 26, 2024

Visit the preview URL for this PR (updated for commit be8f7e8):

https://dart-dev--pr4231-vale-implementation-rsgx3g06.web.app

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.

None yet

4 participants