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

Using Vale in CI for spellcheck #4423

Merged
merged 24 commits into from
Mar 18, 2023
Merged

Using Vale in CI for spellcheck #4423

merged 24 commits into from
Mar 18, 2023

Conversation

MAX-786
Copy link
Contributor

@MAX-786 MAX-786 commented Feb 22, 2023

fixes #4416

@netlify
Copy link

netlify bot commented Feb 22, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit eb224fc
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64161527351b3a0008eabb54
😎 Deploy Preview https://deploy-preview-4423--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 settings.

@cypress
Copy link

cypress bot commented Feb 22, 2023

Passing run #4555 ↗︎

0 489 20 0 Flakiness 0

Details:

Merge branch 'master' into use-vale-in-ci
Project: Volto Commit: eb224fc059
Status: Passed Duration: 12:49 💡
Started: Mar 18, 2023 7:50 PM Ended: Mar 18, 2023 8:03 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@stevepiercy
Copy link
Collaborator

All the files in styles/Microsoft/ should not be checked in. Please remove them and ensure that the .gitignore entry mirrors that of plone/documentation.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Feb 22, 2023

Sure! was going to that too, thanks!😊

@MAX-786 MAX-786 marked this pull request as ready for review February 22, 2023 19:48
@MAX-786 MAX-786 requested a review from stevepiercy February 22, 2023 19:49
@MAX-786
Copy link
Contributor Author

MAX-786 commented Feb 22, 2023

@stevepiercy should I move the styles folder to the .github folder as it will be less confusing?

@stevepiercy
Copy link
Collaborator

@stevepiercy should I move the styles folder to the .github folder as it will be less confusing?

What makes it confusing? I don't understand.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Feb 22, 2023

IMHO, since it will be out there in root directory and ppl mostly use styles folder for CSS and styling files and also Vale action README also suggest to keep styles folder inside .github. Love to hear your thoughts on this!

@stevepiercy
Copy link
Collaborator

It definitely should not be in a dot-folder, where it might not show up in editors. We want people to contribute to it. It's fine where it is, and is consistent with plone/documentation and all other projects where generated files are kept at the root of the project. This makes it easier to exclude them for packaging and releasing as well.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Feb 22, 2023

Oh i see they are on root directory, thanks i got it :)

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.

This looks pretty good, but we need to test it in CI before merging. See my review comments, and if that does not work, check the GitHub Actions docs for more ideas.

I think @fredvd also had this chicken-and-egg situation, where he could not test CI until the PR was merged, but merging was too soon. He might have some tips.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.vale.ini Outdated Show resolved Hide resolved
news/4423.feature Outdated Show resolved Hide resolved
styles/Vocab/Plone/accept.txt Outdated Show resolved Hide resolved
@fredvd
Copy link
Member

fredvd commented Feb 22, 2023

@stevepiercy The test/job does run. See for example https://github.com/plone/volto/actions/runs/4246317905/jobs/7382971562

the vale action now is set to always succeed though, check the documentation to make it fail when it finds errors. Or is that not the intention? It doesn't report any findins now in the result.

We could maybe add that link to the report to the summary output of the job.

@MAX-786 the documentation and vale jobs now run in parallel. Is that your intention? So Vale is not testing anything from the generated docs, but can only check the source files.

If you want to have one report with output, it would be nicer to run vale after the documentation build and linkcheck as an extra step. Then you'd also have only one checkout and one report/job output to test for warnings/problems etc.

@stevepiercy
Copy link
Collaborator

We shall not require Vale success at this time. There are way too many words that need to be added to the accept/reject files before this can happen. Instead we want it to provide feedback to the contributors and reviewers for further action.

For example, when I run make vale in plone/documentation, I get helpful feedback about misspelled words and style improvements, which I often incorporate into my PRs.

Vale works on Markdown source: https://vale.sh/docs/topics/scoping/#markdown

The Vale runner in CI is not configured to find any of the source files from checkout, which may be a "success" or a "lack of failure":
https://github.com/plone/volto/actions/runs/4246317905/jobs/7382971562#step:4:26

The Vale runner needs to be configured correctly. I am not sure how to do that. Let's let @MAX-786 give it a shot, unless you @fredvd happen to know what that is.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Feb 23, 2023

If you want to have one report with output, it would be nicer to run vale after the documentation build and linkcheck as an extra step.

@fredvd I tried doing it, if this isn't working I can revert it back to before.

The Vale runner needs to be configured correctly. I am not sure how to do that

@stevepiercy as I mentioned in the above comment I guess we can an optional input to configure vale. Let me know your thoughts :)

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.

Remember to check the result of the Documentation build in GitHub Actions.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@stevepiercy
Copy link
Collaborator

I cancelled the Documentation workflow because it appeared to have stalled after 43 minutes. I then restarted it. We'll see....

.github/workflows/docs.yml Outdated Show resolved Hide resolved
@sneridagh
Copy link
Member

@stevepiercy status?

@stevepiercy
Copy link
Collaborator

@stevepiercy status?

#4423 (review)

@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 16, 2023

🥹Apologies for my recent absence due to dengue fever. Thank you for your patience and support. I'll get back to this PR now. Agaain sorry for the delay!

@stevepiercy
Copy link
Collaborator

@MAX-786 I'm glad your feeling better and back. Please let me know if you have any questions. It looks like the docs CI build still stalls and never completes.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 16, 2023

@MAX-786 I'm glad your feeling better and back. Please let me know if you have any questions. It looks like the docs CI build still stalls and never completes.

Thanks @stevepiercy , I'll let you know asap ....

@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 17, 2023

Now, after trying many times, vale GitHub action works well on my local machine! ( using nektos/act to run github actions locally).
let's wait and see if everything goes well here or not :).

@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 17, 2023

Linkcheck & html build is failing! Skipping both for now.

@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 17, 2023

We are getting some Findings NOW! :)
https://github.com/plone/volto/runs/12093413147

But, it is giving all findings before getting to Finding Report
And it is not showing all the findings in the Report

@stevepiercy I am trying to find a way to get more structured output, please let me know if you know a way to do it.
Thanks 😊

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.

This looks good, with the exception to revert the commented workflows.

.github/workflows/docs.yml Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
.vale.ini Show resolved Hide resolved
MAX-786 and others added 3 commits March 18, 2023 23:17
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@MAX-786 MAX-786 requested a review from stevepiercy March 18, 2023 17:54
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.

w00t! We have spelling and style check! Thank you!

@stevepiercy
Copy link
Collaborator

One more run after merging master to this PR. If it passes, I'll merge.

@stevepiercy stevepiercy merged commit d3865b3 into master Mar 18, 2023
@stevepiercy stevepiercy deleted the use-vale-in-ci branch March 18, 2023 21:25
@stevepiercy stevepiercy added this to the Plone 6.0 milestone Mar 18, 2023
@MAX-786
Copy link
Contributor Author

MAX-786 commented Mar 19, 2023

it passed! 🥹🥳

sneridagh pushed a commit that referenced this pull request Mar 21, 2023
Co-authored-by: Steve Piercy <web@stevepiercy.com>
sneridagh added a commit that referenced this pull request Mar 21, 2023
Co-authored-by: Mohammad Hussain <99530996+MAX-786@users.noreply.github.com>
Co-authored-by: Steve Piercy <web@stevepiercy.com>
sneridagh added a commit that referenced this pull request Apr 5, 2023
* master: (67 commits)
  Add current page parameter to the route in the listing and search block pagination (#4159)
  Fixed wrong localization on password reset page(#4656) (#4657)
  Release notes for 16.19.0 (#4655)
  Razzle upgrade notice in upgrade guide (#4641)
  Release generate-volto 7.0.0-alpha.3
  Update to latest Razzle - needed since #3997. This fixes the duplicated Razzles issue (#4640)
  3092 improve spellcheck (#4633)
  developer process for first time contributing (#4617)
  Trigger CI on pull_request event (#4629)
  Pining of `pydata-sphinx-theme` and `sphinx-book-theme`, CI is complaining. (#4626)
  Set sameSite in `18N_LANGUAGE` cookie (#4627)
  Update simple-git (#4546)
  DefaultView (blocks disabled): Show field name as tip on hover of label (#4598)
  Fix regexp that checks valid URLs and improve tests (#4601)
  Release 17.0.0-alpha.3
  Update changelog relese 16.18.0 (#4596)
  InternalURl helper method should incorporate externalRoutes settings … (#4560)
  Update message Add-on control panel (#4574)
  Fix video warnings link errors (#4578)
  Using Vale in CI for spellcheck (#4423)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use Vale for spellcheck
4 participants