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

Adding "Symbols" rule #413

Merged
merged 1 commit into from
Aug 23, 2023
Merged

Adding "Symbols" rule #413

merged 1 commit into from
Aug 23, 2023

Conversation

aireilly
Copy link
Member

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

🎊 Navigate the preview: https://redhat-documentation-vale-at-red-hat-413.surge.sh 🎊

@github-actions
Copy link

Click here to review and test in web IDE: Contribute

@aireilly aireilly changed the title Adding "Symbols" rule WIP - Adding "Symbols" rule Jan 26, 2023
@aireilly aireilly changed the title WIP - Adding "Symbols" rule Adding "Symbols" rule Jan 26, 2023
@aireilly
Copy link
Member Author

aireilly commented Feb 1, 2023

@jhradilek WDYT?

@jhradilek
Copy link
Contributor

Hi @aireilly, I am terribly sorry for my late reply, I did not notice the mention.

This is a great addition and the output is very helpful. I ran this on my large documentation set and the only error I found is that the rule incorrectly reports unset AsciiDoc attributes. You can reproduce it by adding the following in .vale/fixtures/RedHat/Symbols/testvalid.adoc:

:!attribute:
:attribute!:

The only other questionable case is in keyboard macros where I am not sure what is our official guidance for key combinations:

kbd:[Ctrl + a]

@jhradilek
Copy link
Contributor

I spoke too early. Can we please add an exception for C++?

@aireilly
Copy link
Member Author

Excellent feedback @jhradilek I will review and incorporate your suggestions. I need to test against some other repos too I think.

@jhradilek
Copy link
Contributor

I was looking up documentation on the include statement and accidentally found one more problematic case. According to the documentation, prefixing an include statement with a backslash is a valid way to escape it. Consequently, the following is incorrectly reported by Vale:

\include::example.adoc[leveloffset=+1]

I am not aware of situations where we actually use this in our documentation and cannot imagine it is common outside of a verbatim block, but thought I should report it.

@aireilly
Copy link
Member Author

@jhradilek This one is ready for review finally. Note asciidoctor/asciidoctor#1208

C++ is tricky for AsciiDoc processors. There is a builtin {cpp} attribute for this.

@jhradilek
Copy link
Contributor

Hi @aireilly, I just tested it on my production data and noticed that the current version reports + symbols in list continuations and inside code blocks. To reproduce, add the following at the end of .vale/fixtures/RedHat/Symbols/testvalid.adoc:

. Lists and procedure steps can use a list continuation:
+
[literal,subs="+quotes"]
....
$ *diff --unified a b*
--- a  2023-08-22 19:02:22.940992549 +0100
+++ b  2023-08-22 19:03:12.244543569 +0100
-hello
+goodbye
....

Running vale on it produces the following output:

$ vale testvalid.adoc

 testvalid.adoc
 15:1   suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 19:38  suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 20:1   suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 20:2   suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 20:3   suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 20:38  suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 22:1   suggestion  Do not use the '+' symbol.  RedHat.Symbols 

✔ 0 errors, 0 warnings and 7 suggestions in 1 file.

I think most of these could be avoided if you just ignore + symbols at the beginning of a line and their duplicates.

@aireilly
Copy link
Member Author

I might just remove the + checking altogether. It's used quite a bit in AsciiDoc.

@aireilly
Copy link
Member Author

Oh actually if we remove scope:raw those list continuations are ignored. OK, this is working as it should I think. I'll do some more tests against RHEL and OCP to verify.

@jhradilek
Copy link
Contributor

I didn't think about changing the scope, that's a good point. Just please be aware that Vale's understanding of what the sentence scope is differs from what is intuitive. It will for example not report problems in bullet points, try changing the contents of the testinvalid.adoc file as follows:

Don't use this!
Don't use this & that
This + that is not OK.

* `this` is a bullet point. Don't use this!
* Don't use this & that.
* This + that is not OK.

Vale sadly ignores the last three lines:

$ vale testinvalid.adoc 

 testinvalid.adoc
 2:16  suggestion  Do not use the '&' symbol.  RedHat.Symbols 
 3:6   suggestion  Do not use the '+' symbol.  RedHat.Symbols 
 5:43  suggestion  Do not use the '!' symbol.  RedHat.Symbols 

✔ 0 errors, 0 warnings and 3 suggestions in 1 file.

@jhradilek
Copy link
Contributor

jhradilek commented Aug 23, 2023

Actually, looking at the line numbers again, in addition to what I stated above there seems to be another problem similar to what we observed in PR #366.

@aireilly
Copy link
Member Author

aireilly commented Aug 23, 2023

@jhradilek OK, can't spend any more time on this. Vale/AsciiDoc are too finicky around + symbol. I'm dropping it from this PR.

Interesting re:sentence scope. That's quite counterintuitive IMO. I may raise it as a bug on the Vale project.


= Symbols

Do not use symbols such as ampersand (`&`), exclamation point (`!`), or plus symbol (`+`) in original body copy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Do not use symbols such as ampersand (`&`), exclamation point (`!`), or plus symbol (`+`) in original body copy.
Do not use symbols such as ampersand (`&`) or exclamation point (`!`) in original body copy.

@jhradilek
Copy link
Contributor

Thanks for all your work on this! Looks good to me.

@aireilly aireilly disabled auto-merge August 23, 2023 16:17
@aireilly aireilly merged commit 946286f into redhat-documentation:main Aug 23, 2023
3 checks passed
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.

2 participants