plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 574 forks source link

improved spellcheck #5056

Closed chirayu-humar closed 8 months ago

chirayu-humar commented 8 months ago

Closes - https://github.com/plone/documentation/issues/1190 Fixex "#1190"

accept reject entry_to_change_log_file

netlify[bot] commented 8 months ago

Deploy Preview for volto ready!

Name Link
Latest commit 81f68c5e8be23256c7245671627c96d629dc1f9b
Latest deploy log https://app.netlify.com/sites/volto/deploys/64d888ffa7c8d10008d373d9
Deploy Preview https://deploy-preview-5056--volto.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

chirayu-humar commented 8 months ago

Hello @stevepiercy,

I hope you're doing well! I have prepared a draft pull request (1190) that improves spellcheck. Since you are the maintainer familiar with this area, I would greatly appreciate your expert feedback and review.

This pull request aims to address issue 1190 and adds functionality to improve the user experience.

Please let me know if you have time to take a look at it whenever you get a chance. I'm open to any suggestions, and I'm eager to make any necessary adjustments to align with the project's guidelines and standards.

Thank you in advance for your time and consideration.

Best regards, @chirayu-humar ,

stevepiercy commented 8 months ago

@chirayu-humar sorry, this PR is the exact opposite of what we need. See:

https://6.docs.plone.org/contributing/documentation/setup-build.html#vale

Plone documentation uses a custom spelling dictionary, with accepted and rejected spellings in styles/Vocab/Plone.

Words in reject.txt are rejected, whereas those in accept.txt are accepted. The existing entries in reject.txt should be rejected. Please do not remove them.

chirayu-humar commented 8 months ago

ok, i feel now i understood, i have to include "JavaScript", "NodeJS" in accept.txt and "Javascript", "Js", "node.js" and "node" etc in reject.txt, am i write ? but in master branch, list inside accept is :- plone.restapi,plone.volto , npm, Plone, Razzle, RichText, Volto, Zope, JavaScript, NodeJS.

which already includes "JavaScript" and "NodeJS" and

list inside reject.txt is :- node, nodejs, javascript, js, Javascript.

which includes all unfavourable words which you mentioned in requirements above then what needs to be improved here, we can just add some more unfavourable words to reject.txt like "node.js" and "Js" etc. please convey your thoughts on this, i will be very happy to proceed accordingly.

stevepiercy commented 8 months ago

Let's take a step back. Vale already has a built-in dictionary against which it checks spelling of thousands of words.

Vale will reject some words that we want to accept, such as Volto. We put such words in accept.txt.

Similarly Vale might accept words that we want to reject. This includes improper casing, such as "Javascript", and incorrectly spelled words. We put such words in reject.txt.

That is how we customize spell checking.

To run spell checking, you can call make vale.

You can also call Vale directly with specific commands to improve the spelling of a single file or directory of files. vale -h will display Vale's help.

chirayu-humar commented 8 months ago

i studied vale.sh documentation whose link you provided, and i looking at present code in both accept.txt and reject.txt, i feel they both are working properly i was not able to test because both "make vale" and "vale -h" was not running in volto directory, i even tried installing vale on my local system using docker but i failed to do so, due to lack of knowledge to implement .yml files. instalation process for ubuntu is not present on documentation on vale.sh.

Now i feel that should be working fine, both "NodeJS" and "JavaScript" are present in accept.txt hence both will be accepted and "Javascript", "javascript" and "js" will get rejected because these 3 are present in reject.txt.

now i am unclear about lins which you mentioned in issue https://github.com/plone/documentation/issues/1190, which are :-

JavaScript, instead of javascript / Javascript / js
NodeJS, instead of nodejs / Node.js / node

if we replace (javascript / Javascript / js) with JavaScript in reject.txt then it will start rejecting "JavaScript" which is a mistake i made previously due to misunderstanding. please give some clarity on your Requirements, i mean to spacify what did the organization mean from the above two lines, because code is already working fine so how can we improve spelling consistancy using above two lines, according to me spellings will already going to be consistent because code will only going to accept "JavaScript" and "NodeJS".

I do not have much experience with vale.sh but i am ready to work and solve issue but i am stuck because of lack of clarity on Requirements. One prospact of looking at the situation, as much as i am able to understand is, we can add some more words with improper casing like "NodeJs" or "Node.JS", it will also improve consistency.

i observed some rules related to consistency in documention which are like this :- https://vale.sh/docs/topics/styles/#consistency, but not able to figure out how to apply this.

i found some more rules which will git flaxibility in acceptance :-

first
[pP]y.*\b
third
(?i)MongoDB
[Oo]bservability

but above rules will only increase the scope of acceptance level, which will worsen the consistency, hence are useless.

i request to please give some clarity on what kind of changes are required so that i can proceed.

stevepiercy commented 8 months ago

I'm sorry, I don't know how to explain further. The correct spelling of "JavaScript" is exactly that, not "javascript", "Javascript", or "js". That's why the first is in accept.txt and the other three are in reject.txt.

These serve as examples of how to find other words that should be accepted or rejected throughout the documentation and added to those two files.

When you run make vale, there should be a lot of spelling warnings that could benefit from proper spelling and casing configuration.

I don't think you can use regular expressions in these two files, but you can use ignore files and filters in the configuration.

chirayu-humar commented 8 months ago

have installed vale and tried running command "make vale" but there is no such rule named "vale" but there is one which i identified, which is named "docs-vale".

stevepiercy commented 8 months ago

Ah, yes, for non-documentation repos, we use the prefix docs- for make commands. Sorry about that. Please use make docs-vale instead, and let me know.

chirayu-humar commented 8 months ago

hi @stevepiercy, fortunately at last command "make docs-vale" ran successfully, on the bassis of suggestions it gave to me in output i have commited a chage to pull request, added "plone", "volto" and "razzle" to reject.txt file, because these 3 are wrong casing configurations and there write once are already written in accept.js so they are not needed to be added again,

but now, i just request you sir, please review my pull request and guide whether my approach is write ?, i beleave that i have to put all the words after "instead" into reject.txt and all words before it into accept.txt, for example :-

90:33   error       Use 'for example' instead of 'e.g.'.
85:22   error       Use 'isn't' instead of 'is not'.
39:14  suggestion  Consider using 'many' instead of 'multiple'.

in above three examples, i will add ("for example", "isn't" , "many" ) to accept.js and ( "e.g.", "is not" , "multiple" ) to reject.js is this approach correct ? or i am wrong this time ? please explain , if the approach is correct i will try to add more names to both accept.txt and reject.txt and will make a final commit and

screen shots of current changes :- image

Note:- by mistake i have added node.js to reject.txt file twice, i was attempting to add "Node.js" but by mistake vs code automaticaly adjusted the case after enter and i also did not paid attention at that time so i will correct that in next commit.

stevepiercy commented 8 months ago

I would have to see the usage to be sure, but in general I agree with the two errors. I think it is safe to make the changes in the docs to silence these errors from Vale.

However, the suggestion depends on context. Keep in mind that Vale is not perfect, and "multiple" might be better than "many". English grammar is needlessly complex. Can you provide more context or its usage?

chirayu-humar commented 8 months ago

hello @stevepiercy, sorry for tagging you again, i was unsure about, whether you have been notified or not about my last commnent for review, answers to your questions are as follows:-

before adding these entries, vale reported like this :- use "Volto" instead of "volto" use "Plone" instead of "plone" use "NodeJS" instead of "Nodejs" use "Razzle" instead of "razzle"

after adding entries, when i ran command "make docs-vale" there were lines in output :-

84:63 error Use 'Plone' instead of 'plone'. 20:38 error Use 'Volto' instead of 'volto'. 127:69 error Avoid using 'node'. 280:19 error Use 'NodeJS' instead of 'Nodejs'. 58:42 error Use 'Razzle' instead of 'razzle'.

so, i we can say that yes; even after adding these terms to reject.txt and accept.txt, errors still persists, Vale still reports that their usage in the docs should be corrected. yes.

comming to third question, i did not correct their usage in docs, but my cross question to you sir, do i need to correct their usage in docs along with adding them to reject and accept ? if that is the case then i am ready to do that also, please give instructions to proceed.

stevepiercy commented 8 months ago

so, i we can say that yes; even after adding these terms to reject.txt and accept.txt, errors still persists, Vale still reports that their usage in the docs should be corrected. yes.

You configure Vale with terms in accept.txt and reject.txt, then correct the instances in the docs both accordingly and depending on their context. Vale is pretty good about avoiding code and inline literals, but it can make a mistake. Code examples in the docs with <javascript> should not be reported as misspellings. When reviewing the recommendations that Vale makes, use your best judgment to update the docs. In any case, changes will be reviewed by a maintainer.

comming to third question, i did not correct their usage in docs, but my cross question to you sir, do i need to correct their usage in docs along with adding them to reject and accept ?

Yes. The whole point of running a spell and grammar checker is to catch misspellings and grammar mistakes for you to correct in the docs. A PR with additions to the configuration and their corresponding corrections in the docs is expected.

For this PR, focus on only a few new terms, then move it out of Draft status for final review.

For Volto only, Vale currently reports:

βœ– 822 errors, 378 warnings and 1198 suggestions in 93 files.

I would be overjoyed to get 1% of those errors, warnings, and suggestions resolved in one PR.

chirayu-humar commented 8 months ago

i have recently made 2 commits into this forked repo in spell-check-update branch, after resolving some errors, majorly i have resolved 2 types of errors i have resolved:-

@stevepiercy, please review the lattest commit, ready to resolve some more errors if every thing is fine till now, waiting for response

chirayu-humar commented 8 months ago

i have updated the this pull request (forked repo) with the upstream, because a commit was made into master branch of upstream by @stevepiercy . please review the changes of lattest commit

chirayu-humar commented 8 months ago

not included "http" and "https" and "css" in reject.txt but removed from accept.txt because it may case problems with URLs

chirayu-humar commented 8 months ago

sorry, i requested for review before pushing the commit, please review the lattest commit named "changed the left chages". removed "http" and "https" from accept.txt but not included them to reject.txt because i thought it could cause problems.

stevepiercy commented 8 months ago

It's difficult for me to follow you. Thus let's focus only on the Vale output. For example:

 docs/source/deploying/apache.md
 12:58   error  Avoid using 'api'.                        Vale.Avoid       
 12:107  error  Punctuation should be inside the quotes.  Microsoft.Quotes 
 12:146  error  Did you really mean 'http'?               Vale.Spelling    
 12:154  error  Did you really mean 'https'?              Vale.Spelling

This tells me to look at that file and line to get the context of the usage, and I can make a correct decision. See https://github.com/plone/volto/pull/5056/commits/c4136b0e13cdb668c8a4ad74273ad2b056f000ec to see what I did to fix all 4 of these issues.

docs/source/deploying/pm2.md
 10:22  error       Avoid using 'node'.                 Vale.Avoid             
 12:63  error       Avoid using 'js'.                   Vale.Avoid             
 18:53  error       Did you really mean 'repo'?         Vale.Spelling          
 57:49  error       Use 'that's' instead of 'that is'.  Microsoft.Contractions 
 58:38  suggestion  'ZEO' has no definition.            Microsoft.Acronyms     

The first two errors are because Vale considers a period as a boundary character. I tried using a regular expression in accept.txtβ€”Node\.jsβ€”to no effect. However there is a workaround:

{term}`Node.js`

It's verbose but correct. It is defined in the Glossary, so it will resolve to it and pass spellcheck. Alternatively we could replace all instances of "Node.jd" as an inline literal, but I think that because it is a registered trademark, it should not be an inline literal.

Here's my commit that resolves most of these issues. 6f06daa

stevepiercy commented 8 months ago

I created an issue to get help for that Node.js term thing. It bugs me that we have to use a workaround for it. https://github.com/errata-ai/vale/issues/666

Meanwhile, it is fine to ignore the grammar suggestions.

chirayu-humar commented 8 months ago

so shall i convert this pull request from "draft" to "ready for review" ?

stevepiercy commented 8 months ago

@chirayu-humar I resolved most of the issues, but not all. When you resolve the remaining issues, specifically with "Node.js", then change it to "ready for review". Thank you!

stevepiercy commented 8 months ago

@chirayu-humar I just got a very helpful response from the maintainer of Vale. Would you please see their response, and update according to their guidance: https://github.com/errata-ai/vale/issues/666#issuecomment-1670457942 ? Thank you!

chirayu-humar commented 8 months ago

i have merged all the changes made by @stevepiercy but reverted ( Node.js to Node.js in line 10 and 12 of "deployment/pm2.md" ) because i could be handled using changes suggested by vale maintainer, and obviously, i have implemented them also please review my lattest commit, if every thing is alright then may i convert the draft into "ready for review" ?

chirayu-humar commented 8 months ago

@stevepiercy, can i convert this pull request from draft to "ready to review", please comment on this so that i can proceed with that, Thanks allot for guiding till now.

stevepiercy commented 8 months ago

@chirayu-humar you can do that now. It does mean, after all, "ready to review" not "MERGE ME NOW, RAWR!" πŸ˜‰

stevepiercy commented 8 months ago

@chirayu-humar instead of going through another review cycle, I decided it would be easier to demonstrate what is involved with configuring Vale, especially from the excellent lesson I learned from its maintainer. Please take a look at the configuration and spellings.

I don't think I want to punish a core maintainer by asking them to review the hundreds of docs corrections in this PR, as they are trivial, but at the same time necessary. I'm going to approve this PR as is, and leave it to a core maintainer to merge. @sneridagh @davisagli

Before:

βœ– 822 errors, 378 warnings and 1198 suggestions in 93 files.

After

βœ– 602 errors, 380 warnings and 1219 suggestions in 94 files.

I opened an issue for how to handle "plone" and "volto" in MyST labels.

Once this PR is merged, then I will update the configuration of Vale in the main Documentation repo.