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

Escape +/- key in constant descriptions to avoid issues when generating RST #82697

Conversation

transistasis
Copy link

Hey all, this is my first pull request here.

Problem Description

This PR fixes the problems raised in #8127. The issues stem from the + and - characters used in the KEY_PLUS and KEY_MINUS enumeration descriptions not being escaped after the make_rst.py script is run. This results in the + and - characters being interpreted as bullet lists, rather than characters, as can be seen here.

Problem Solution

The cleanest solution that I can think of is to add additional escape logic to the escape_rst method in make_rst.py for the + and - characters by searching for the strings "+ key" and "- key". This will allow the KEY_PLUS and KEY_MINUS enumeration descriptions to display correctly after generating a .rst file from @GlobalScope.xml. This solution will be transparent to all of the other .xml files in the project.

I looked at trying to escape these characters in the same way that the * is handled, but found that this can break bullet lists in .rst files generated from other .xml files, such as EditorCommandPalatte.xml. As a result, the proposed solution is the cleanest way that I can think of to fix the problem.

Results

Upon diffing the generated _build/rst folders before and after applying the fix, only class_@globalscope.rst is modified:
image

Please let me know if someone has a cleaner solution!

…inferred as bullet lists for the KEY_PLUS and KEY_MINUS enumerations after generating an .rst file from @GlobalScope.xml
@transistasis transistasis requested a review from a team as a code owner October 3, 2023 00:24
@YuriSizov YuriSizov added this to the 4.2 milestone Oct 3, 2023
@YuriSizov YuriSizov changed the title RST generation escape character fix Escape +/- key in constant descriptions to avoid issues when generating RST Oct 3, 2023
@AsperTheDog
Copy link

AsperTheDog commented Oct 3, 2023

It is also possible to fix this by editing doc/classes/@GlobalScope.xml in the godot repo. Specifically by subsituting the - in line 2171 (description definition for KEY_MINUS) with - and the + in line 2165 (description definition for KEY_PLUS) with +

This is how cases like < in line 2216 (description definition for KEY_LESS) and > in line 2222 (description definition for KEY_GREATER) are handled right now.

I personally think this is cleaner since it does not add any additional code and it mimics the approach made in other parts of the xml file

@RedMser
Copy link
Contributor

RedMser commented Oct 3, 2023

This would pair well with #67037 , and with both merged we could also escape * the same way.

@transistasis
Copy link
Author

@AsperTheDog I gave it a shot, but I didn't end up being successful. I did the following tests without any of the make_rst.py changes included in this PR:

  1. Try to brute force the escape sequence for KEY_PLUS by using &#92;&#43; or \&#43; in @GlobalScope.xml
  2. Try to handle KEY_MINUS the same way that KEY_LESS is handled by using &#45; in @GlobalScope.xml

The first approach didn't work since the \ character is also escaped in the escape_rst method of make_rst.py, leading to an output of \\+ key. being generated in the associated .rst file.

The second approach didn't work since it will generate an unescaped - character in the generated .rst file, leading to an output of - key., which is what is currently happening in master. The use of &lt; for KEY_LESS works fine since this character doesn't need to be escaped in the generated .rst file.

I've included a screenshot for completeness:
image

Do you know if I'm missing something? To your point, I would prefer not to add any additional code to make_rst.py, so I'd really like this alternative approach to work. 🙏

@AsperTheDog
Copy link

AsperTheDog commented Oct 3, 2023

@transistasis it seems like it does not work as I expected. My bad. This issue is only present in the online docs, and not in the local docs of the engine. To test it looked as it should I compiled the engine and ran it with --doctool. Since the docs appeared to be correct I assumed the fix was working correctly.

So the approach I mention doesn't work.

@transistasis
Copy link
Author

@AsperTheDog It's all good, thanks for taking another look at it!

If you have any other ideas, let me know and I'll try them out. 🤘😼

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 10, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Nov 10, 2023

NumPad keys' descriptions avoid this by starting with a word.

Add (+) key on the numeric keypad.

I think this can be done here as well, for all related descriptions. The proposed hack I don't think is a good idea, as it's based on a fragile relationship with the current phrasing, and a more general solution is not possible, because the format is currently ambiguous.

@KoBeWi KoBeWi removed this from the 4.3 milestone Aug 4, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Aug 4, 2024
@transistasis transistasis closed this by deleting the head repository Aug 16, 2024
@AThousandShips AThousandShips removed this from the 4.x milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants