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

Fixing vale warning #2394

Merged
merged 50 commits into from
Oct 17, 2023
Merged

Fixing vale warning #2394

merged 50 commits into from
Oct 17, 2023

Conversation

clatapie
Copy link
Contributor

@clatapie clatapie commented Oct 6, 2023

Vale errors have appeared and need to be fixed:
image

@ansys-reviewer-bot
Copy link
Contributor

Thanks for opening a Pull Request. If you want to perform a review write a comment saying:

@ansys-reviewer-bot review

@github-actions github-actions bot added the BUG label Oct 6, 2023
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

Merging #2394 (db6da8e) into main (5040f12) will decrease coverage by 6.05%.
Report is 3 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2394      +/-   ##
==========================================
- Coverage   87.93%   81.88%   -6.05%     
==========================================
  Files          44       44              
  Lines        8453     8453              
==========================================
- Hits         7433     6922     -511     
- Misses       1020     1531     +511     

doc/.vale.ini Outdated Show resolved Hide resolved
@germa89
Copy link
Collaborator

germa89 commented Oct 6, 2023

I'm not sure why this happens. First time I saw it was in #2384

I suspect it is a bug in vale, because nothing really changed on our side, and they released a new patch version 16h ago: https://github.com/errata-ai/vale/releases/tag/v2.29.2

Pinging @Revathyvenugopal162 @RobPasMue

@Revathyvenugopal162
Copy link
Contributor

@clatapie try adding mapdl and Mapdl in the vale accept.txt file. (may be the conflict between codespell and vale)

@germa89
Copy link
Collaborator

germa89 commented Oct 6, 2023

I'm not sure why this happens. First time I saw it was in #2384

I suspect it is a bug in vale, because nothing really changed on our side, and they released a new patch version 16h ago: https://github.com/errata-ai/vale/releases/tag/v2.29.2

Pinging @Revathyvenugopal162 @RobPasMue

Thinking it more thoughtful, because the latest vale version was failing silently, we might have been having this issue for a long time.

@clatapie try adding mapdl and Mapdl in the vale accept.txt file. (may be the conflict between codespell and vale)

It is already in accept.txt:

However the problem is that vale is analysis text inside code blocks, references, inline code, etc.

.vale.ini Outdated Show resolved Hide resolved
@clatapie
Copy link
Contributor Author

clatapie commented Oct 9, 2023

An identical issue is raised in the PyMAPDL-Examples repository.

clatapie and others added 3 commits October 11, 2023 15:54
Co-authored-by: Revathy Venugopal <104772255+Revathyvenugopal162@users.noreply.github.com>
@clatapie clatapie self-assigned this Oct 11, 2023
Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

Overall I'm happy with this PR. Thank you for taking care of it.

Couple of remarks:

  • The file .vale.ini has change completely, but actually only couple of lines have changed. I would suggest you to revert back that file to the version in main, and then do in the desired lines the changes you want. I would like to no "alter" the github history in the lines not actually changed.
  • Changes in the headings/titles in troubleshoot.rst. I presume you did it because you want to keep consistency with other documents. I appreciate that. But because of that, we ran out of section headers. I rather change consistency but have headers and reference there. Furthermore, the ubuntu section I reference it a lot in projects and externally.

doc/.vale.ini Outdated Show resolved Hide resolved
doc/.vale.ini Outdated Show resolved Hide resolved
doc/source/user_guide/troubleshoot.rst Show resolved Hide resolved
doc/styles/Vocab/ANSYS/accept.txt Outdated Show resolved Hide resolved
@clatapie clatapie requested a review from germa89 October 13, 2023 10:48
Copy link
Collaborator

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

Small comment.

Ideally with the regex we are using we wouldn't have to make so many links changes (from two lines to one).
I am happy to keep those changes because it helps VSCode to highlight them in the editor.

Good work!

doc/source/user_guide/mapdl.rst Show resolved Hide resolved
@clatapie clatapie enabled auto-merge (squash) October 13, 2023 12:35
@github-actions
Copy link
Contributor

Hello! 👋

Your PR is changing the image cache. So I am attaching the new image cache
in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore
you will see the actions showing in their status Expected — Waiting for status to be reported. Do not worry.
You commit workflow is still running here 😄

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. 🤓

@github-actions
Copy link
Contributor

Hello! 👋

Your PR is changing the image cache. So I am attaching the new image cache
in a new commit.

This commit does not re-run the CICD workflows (since no changes are made in the codebase) therefore
you will see the actions showing in their status Expected — Waiting for status to be reported. Do not worry.
You commit workflow is still running here 😄

You might want to rerun the test to make sure that everything is passing. You can retrigger the CICD sending an empty commit git commit -m "Empty comment to trigger CICD" --allow-empty.

You will see this message everytime your commit changes the image cache but you are not attaching the updated cache. 🤓

@clatapie clatapie merged commit 32743f0 into main Oct 17, 2023
26 checks passed
@clatapie clatapie deleted the fix/codestyle branch October 17, 2023 08:04
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.

3 participants