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

Force doc style check with Vale before building the doc #689

Merged
merged 11 commits into from
Dec 15, 2022

Conversation

PProfizi
Copy link
Contributor

@PProfizi PProfizi commented Dec 14, 2022

Vale had been integrated to the repository but only within a manually triggerable workflow.
This PR calls this workflow in the regular CI as a check step before building the documentation.
https://vale.sh/docs/vale-cli/overview/

Install Vale on Windows using choco (also explains how to install chocolatey):
https://docsy-site.netlify.app/docs/vale/install-vale/
Also install Sphinx using choco (required to treat .rst files):
choco install sphinx

How to integrate it to PyCharm as a tool:
https://vale.sh/docs/integrations/jetbrains/

@PProfizi PProfizi added the CI/CD Related to CI/CD label Dec 14, 2022
@PProfizi PProfizi self-assigned this Dec 14, 2022
@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #689 (0407368) into master (684d6c5) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
+ Coverage   88.20%   88.23%   +0.02%     
==========================================
  Files          69       69              
  Lines        7760     7760              
==========================================
+ Hits         6845     6847       +2     
+ Misses        915      913       -2     

@PProfizi
Copy link
Contributor Author

PProfizi commented Dec 14, 2022

@PipKat @Revathyvenugopal162 I am having Vale errors I do not know how to fix:

  • It raises a Google.Spacing error on paths/filenames such as: "And.Dpf.bat" saying there should be a space between d and D...
  • It raises a Google.Spacing error for class references such as :class:Model<ansys.dpf.core.model.Model> saying there should be a space between l and M.
    Basically it raises spacing errors as soon as a capital letter follows a period. Do you know how to fix/ignore those?

@PipKat
Copy link
Member

PipKat commented Dec 14, 2022

@PProfizi You want to tag all file names in literal strings, like this And.Dpf.bat to avoid Vale errors.
For class directives, you want to use :class: followed by the name of the class surrounded by single back ticks. Here is an example of what I mean:
image

If you'd like, I can check out your branch and resolve all of the Vale errors.

@PProfizi
Copy link
Contributor Author

PProfizi commented Dec 15, 2022

@PProfizi You want to tag all file names in literal strings, like this And.Dpf.bat to avoid Vale errors. For class directives, you want to use :class: followed by the name of the class surrounded by single back ticks. Here is an example of what I mean: image

If you'd like, I can check out your branch and resolve all of the Vale errors.

Hi @PipKat! Thank you! I've proposed modifications to make the Vale test pass, please tell me if what I did is ok as I am not sure this is the best way to allow for what we want.
Also, I do not know why but I get the errors with :class:... only locally. The GitHub action does not raise them.

@PProfizi PProfizi requested a review from PipKat December 15, 2022 10:59
Comment on lines 70 to 71
Running the DPF Server in a Docker container
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member

@PipKat PipKat Dec 15, 2022

Choose a reason for hiding this comment

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

Suggested change
Running the DPF Server in a Docker container
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Running DPF Server in a Docker container
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```Based on the "Getting started with DPF Server" title earlier, it looks like DPF Server is being used like a product name. Consequently, the "the" isn't necessary.


Running the DPF Server in a Docker container
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

1. Along with the ansys_dpf_server_lin_v2023.2.pre0.zip archive mentioned in :ref:`Installing DPF Server <target_installing_server>`, download the Dockerfile.
2. Copy both the archive and Dockerfile in a folder and navigate into that folder.
1. Along with the ansys_dpf_server_lin_v2023.2.pre0.zip archive mentioned in :ref:`Installing DPF Server <target_installing_server>`, download the ``Dockerfile``.
Copy link
Member

Choose a reason for hiding this comment

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

@PProfizi Is the name of the file Dockerfile? If so, you have written and tagged it correctly. If it is a generic reference, it should be "download the Docker file" with it being two words--a file belonging to or related to Docker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PipKat yes indeed the file is named Dockerfile

@PipKat
Copy link
Member

PipKat commented Dec 15, 2022

@PProfizi You want to tag all file names in literal strings, like this And.Dpf.bat to avoid Vale errors. For class directives, you want to use :class: followed by the name of the class surrounded by single back ticks. Here is an example of what I mean: image
If you'd like, I can check out your branch and resolve all of the Vale errors.

Hi @PipKat! Thank you! I've proposed modifications to make the Vale test pass, please tell me if what I did is ok as I am not sure this is the best way to allow for what we want. Also, I do not know why but I get the errors with :class:... only locally. The GitHub action does not raise them.

Your changes look good to me! I'm not sure what the :class: directive is only causing issues locally. Perhaps the links can only be built as part of the GitHub workflow?

Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@PProfizi PProfizi merged commit f9e6385 into master Dec 15, 2022
@PProfizi PProfizi deleted the ci/integrate_vale branch December 15, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Related to CI/CD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants