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

improved spellcheck #5056

Merged
merged 27 commits into from Aug 14, 2023
Merged

improved spellcheck #5056

merged 27 commits into from Aug 14, 2023

Conversation

chirayu-humar
Copy link
Contributor

Closes - plone/documentation#1190
Fixex "#1190"

  • changed to "NodeJS" and "JavaScript".
  • added "entry of change log file" with name "1190.internal".
  • added screen shots of changed code.

accept
reject
entry_to_change_log_file

@netlify
Copy link

netlify bot commented Aug 4, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 81f68c5
🔍 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 chirayu-humar marked this pull request as draft August 4, 2023 04:45
@chirayu-humar
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Contributor Author

chirayu-humar commented Aug 4, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

chirayu-humar commented Aug 4, 2023

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 plone/documentation#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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

chirayu-humar commented Aug 5, 2023

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.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
styles/Vocab/Plone/reject.txt Outdated Show resolved Hide resolved
styles/Vocab/Plone/reject.txt Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

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
Copy link
Contributor Author

chirayu-humar commented Aug 6, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

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:-

  • added unidentified words to accept.txt
  • updated the perfect nouns like "Volto", "Plone" etc according to accept.txt and reject.txt

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

@chirayu-humar
Copy link
Contributor Author

chirayu-humar commented Aug 7, 2023

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

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Progress! I see you have learned some basics, and there are more things to learn, along with some advanced usage. One tip for you, when you use the formal name of a package, I recommend to always check their website for the proper casing and punctuation of the term. Node.js or Node.js® is one of tricky ones, and nvm and npm break the rules of acronyms.

@@ -1,10 +1,22 @@
async
api
Copy link
Collaborator

Choose a reason for hiding this comment

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

Vale does not warn about acronyms when they are uppercased, such as API. api is neither a word nor an acronym because of its improper casing, thus we want this term in the reject.txt file.

Suggested change
api

backend
css
http
https
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move all three above acronyms—css, http, https—into reject.txt, per previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but http and https are used in URLs, we should not capitalize them, in url these are written like this only for example:- https://google.com and http://facebook.com . adding "http" and "https" to reject.txt will force us to capitalize both in URLs, and that will work but will not look good.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Vale should not warn about URLs as long as they follow the style rules. See errata-ai/vale#320

If "http" or "https" are used literally as terms and not in URLs, then they should either be inline literals:

`http` redirects to `https`

...or uppercased to "HTTP" or "HTTPS".

http
https
JavaScript
LTS
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove only. Acronyms should not cause Vale to emit warnings. Where do you see Vale emit a warning for LTS?

Suggested change
LTS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was an error "108:95 suggestion 'LTS' has no definition." in "For docs/source/getting-started/install.md "

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, let's put LTS in here. I want to make sure that when we add configuration, that we do it for a legitimate reason. Also before adding terms wholesale, we should verify that acronyms follow the pattern recommended by Vale to avoid warnings in the first place. See https://vale.sh/docs/topics/styles/#conditional. In this case, LTS is well known, so I won't fuss over it.

styles/Vocab/Plone/accept.txt Outdated Show resolved Hide resolved
styles/Vocab/Plone/accept.txt Outdated Show resolved Hide resolved
@@ -206,7 +206,7 @@ execute `docker run` will be use to persist the backend server data.

If you are somewhat familiar with Python development, you can also install Plone locally
without using Docker. Check the [backend configuration](../configuration/backend.md) section.
It also has more information on plone.volto.
It also has more information on Plone.Volto.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert. If Vale complains about this term, then add it to accept.txt.

Also the original author should have used an inline literal for this, which I think will avoid a Vale warning. Thus:

Suggested change
It also has more information on Plone.Volto.
It also has more information on `plone.volto`.

@@ -266,7 +266,7 @@ You may choose to install the canary version, which is the latest alpha release,
## Build the production bundle

In production environments, you should build an static version of your (Volto) app. The
app should be run in a node process (because of the {term}`server-side rendering`
app should be run in a NodeJS process (because of the {term}`server-side rendering`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
app should be run in a NodeJS process (because of the {term}`server-side rendering`
app should be run in a Node.js process (because of the {term}`server-side rendering`

@@ -277,12 +277,12 @@ side rendering process.
```
The resultant build is available in the `build` folder.

2. Run the Volto Nodejs process
2. Run the Volto NodeJS process
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
2. Run the Volto NodeJS process
2. Run the Volto Node.js process

```bash
yarn start:prod
```

to run the node process with the production build. You can also run it manually:
to run the NodeJS process with the production build. You can also run it manually:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
to run the NodeJS process with the production build. You can also run it manually:
to run the Node.js process with the production build. You can also run it manually:

@@ -9,7 +9,7 @@ myst:

# Deployment using a node process manager (PM2)

PM2 is a popular and maintained process manager based in node (https://pm2.keymetrics.io/).
PM2 is a popular and maintained process manager based in NodeJS (https://pm2.keymetrics.io/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
PM2 is a popular and maintained process manager based in NodeJS (https://pm2.keymetrics.io/).
PM2 is a popular and maintained process manager based in Node.js (https://pm2.keymetrics.io/).

@chirayu-humar
Copy link
Contributor Author

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

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

Please take another look at my previous review. You missed several comments.

Copy link
Contributor Author

@chirayu-humar chirayu-humar left a comment

Choose a reason for hiding this comment

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

I think, I have gone to all the comments now, waiting for further instructions, any changes to make or any further improvement. Till now, i am was only focussing on errors related to perfect nouns, not with suggestions related to vocabs and grammer, am i required to proceed with that because almost all the erros related to perfect nouns are done. waiting for response

styles/Vocab/Plone/reject.txt Outdated Show resolved Hide resolved
backend
css
http
https
Copy link
Contributor Author

Choose a reason for hiding this comment

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

but http and https are used in URLs, we should not capitalize them, in url these are written like this only for example:- https://google.com and http://facebook.com . adding "http" and "https" to reject.txt will force us to capitalize both in URLs, and that will work but will not look good.

http
https
JavaScript
LTS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

there was an error "108:95 suggestion 'LTS' has no definition." in "For docs/source/getting-started/install.md "

styles/Vocab/Plone/accept.txt Outdated Show resolved Hide resolved
styles/Vocab/Plone/reject.txt Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

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. errata-ai/vale#666

Meanwhile, it is fine to ignore the grammar suggestions.

@chirayu-humar
Copy link
Contributor Author

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

@stevepiercy
Copy link
Collaborator

@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
Copy link
Collaborator

@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: errata-ai/vale#666 (comment) ? Thank you!

@chirayu-humar
Copy link
Contributor Author

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" ?

Copy link
Contributor Author

@chirayu-humar chirayu-humar left a comment

Choose a reason for hiding this comment

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

reviwed all changes and have merged them to local branch. knowingly left "Javascript" and "javascript" in reject.txt, it could be deleted and still code will work file according to vale maintainer but still left because you also ignored this whild personaly making changes, if need to be changes i can make change and push again.

@chirayu-humar
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@chirayu-humar chirayu-humar marked this pull request as ready for review August 11, 2023 05:38
@stevepiercy
Copy link
Collaborator

@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.

Copy link
Collaborator

@stevepiercy stevepiercy left a comment

Choose a reason for hiding this comment

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

I approve myself. 😜

@davisagli davisagli merged commit 9d737e9 into plone:master Aug 14, 2023
27 checks passed
davisagli pushed a commit that referenced this pull request Aug 14, 2023
Co-authored-by: Steve Piercy <web@stevepiercy.com>
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

3 participants