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

Use [codeblock lang=text] more often in class reference #89316

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Mar 9, 2024

Looooook at @dalexeev leaving the dirty work for me, aye? See #89263 .

@Mickeon Mickeon added this to the 4.3 milestone Mar 9, 2024
@Mickeon Mickeon requested a review from a team as a code owner March 9, 2024 16:02
@dalexeev
Copy link
Member

dalexeev commented Mar 9, 2024

I've looked at every [codeblock] in the *.xml files. Here is the file list. Sorry for not marking the lines/method names.

1. Files in which you missed something. Godot Shader Language, JSON, XML, formulas, pseudographics and other. For some snippets, GDScript highlighting may be suitable, I just noted for completeness, we can discuss each case.

  • doc/classes/@GlobalScope.xml
  • doc/classes/CanvasGroup.xml
  • doc/classes/ConfigFile.xml
  • doc/classes/DisplayServer.xml
  • doc/classes/EditorPaths.xml
  • doc/classes/FileAccess.xml
  • doc/classes/InputEventKey.xml
  • doc/classes/ResourceImporterCSVTranslation.xml
  • doc/classes/SpriteFrames.xml
  • doc/classes/StyleBoxFlat.xml
  • doc/classes/VisualShaderNodeColorFunc.xml
  • doc/classes/VisualShaderNodeColorOp.xml
  • modules/webrtc/doc_classes/WebRTCPeerConnection.xml
  • platform/ios/doc_classes/EditorExportPlatformIOS.xml
  • platform/macos/doc_classes/EditorExportPlatformMacOS.xml

2. By the way, I noticed a few other problems. Suggestions:

  • doc/classes/AStarGrid2D.xml - Replace C-style ternary with GDScript one?
  • doc/classes/Basis.xml - Split code and output into two codeblocks.
  • doc/classes/Callable.xml - Fix "hello world " -> "hello world".
  • doc/classes/JSON.xml - Add a trailing comma, replace ## with #, turn off highlighting, image tabs with --->.
  • doc/classes/NodePath.xml - Add ^ for consistency.
  • doc/classes/PackedDataContainer.xml - Split code and output into two codeblocks.
  • doc/classes/PackedDataContainerRef.xml - Split code and output into two codeblocks.
  • doc/classes/RenderingServer.xml - Add a trailing comma, fix weird indentation.
  • doc/classes/VisualShaderNodeCustom.xml - Swap extends with class_nameaccording to style guide.
  • doc/classes/XRInterface.xml - Fix weird indentation.

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 9, 2024

I mean, I could've solved those problems in a separate PR. I've noticed a few of these myself.

doc/classes/Basis.xml - Split code and output into two codeblocks.

I actually would've preferred the opposite! That is, merging the codeblock couple in GlobalScope, as it keeps the example more concise and focused. When I see two codeblocks, I think they're entirely unrelated examples (it also avoids the kinda ugly "Prints" in the middle). Bring the sword and let's play fencing over this.

Open for some examples on why it doesn't feel right to me

Compared to:

doc/classes/XRInterface.xml - Fix weird indentation.

#89282

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 9, 2024

doc/classes/@GlobalScope.xml

Where? I can't find any omissions.

doc/classes/CanvasGroup.xml

I'd be wary. I have seen a few code samples referencing the shader language, but I thought that the GDScript Highlighter handled them fairly nicely. Better than plain text, at least.
Of course the best outcome would be to plug in the GDShader highlighter, but that's for another time.
image

Should we mark them as text anyway, so that they become easier to find later, perhaps? I have also seen others that benefit from the GDScript highlighter, and decided to omit them for the same reason.

@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 9, 2024

Actually, massive addendum to all of this that I forgot to mention. A lot of these codeblocks? They probably shouldn't exist. Some of these are poor documentation handling at its finest, in my book.
Take WebRTCPeerConnection:
image

It's why I want to keep this PR focused and avoid other changes, too.

@Mickeon Mickeon force-pushed the codeblock-highlighting-text-ohmygoodness branch from aeabc5b to 2539a10 Compare March 9, 2024 21:35
@Mickeon Mickeon requested review from a team as code owners March 9, 2024 21:35
@dalexeev
Copy link
Member

dalexeev commented Mar 9, 2024

doc/classes/@GlobalScope.xml

Where? I can't find any omissions.

ease()

@Mickeon Mickeon force-pushed the codeblock-highlighting-text-ohmygoodness branch from 2539a10 to b211d82 Compare March 9, 2024 22:03
@Mickeon
Copy link
Contributor Author

Mickeon commented Mar 9, 2024

Aaah, there you go. Thank you. But you see what I mean? That could easily be reworded another way without the codeblock.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

LGTM. Needs a rebase though :(

@Mickeon Mickeon force-pushed the codeblock-highlighting-text-ohmygoodness branch from b211d82 to 328b007 Compare April 8, 2024 14:18
@Mickeon
Copy link
Contributor Author

Mickeon commented Apr 8, 2024

Rebased after minor conflict.

@akien-mga akien-mga merged commit c3a8da6 into godotengine:master Apr 8, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the codeblock-highlighting-text-ohmygoodness branch April 8, 2024 17:03
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.

4 participants