Add GDScript warning pages #10635
Add GDScript warning pages #10635
Conversation
…T_ONREADY` Set up warnings page Add page for each warning, and move content for `GET_NODE_DEFAULT_WITHOUT_ONREADY`
tetrapod00
added
enhancement
content:new page
There was a problem hiding this comment.
I have no complaints with the overall structure of the warning page. The example you've written already seems quite high quality, and you've matched the tone and style of the manual quite well already. I at least would be happy merging this more or less as-is.
The content reads as overall correct but I'm not a deep expert in GDScript so that will need another review.
I left several suggestions for style and minor structure changes, but I'm not certain about some of them. Feel free to disagree and push back on any of them.
For the manual, unlike the class reference, we currently wrap lines to between 80 and 100 characters, as described here. There are plugins that can do this automatically in your IDE too.
| @@ -0,0 +1,41 @@ | |||
| ``GET_NODE_DEFAULT_WITHOUT_ONREADY`` | |||
| ==================================== | |||
|
|
|||
There was a problem hiding this comment.
| Description | |
| ----------- | |
Perhaps?
| The warning that appears when a node's default value is set to a location in the scene tree without using the ``@onready`` annotation. | ||
| The message will say something like: | ||
|
|
||
| .. code-block:: none |
There was a problem hiding this comment.
It's unfortunate that the error here is wider than the screen and requires scrolling with a code block. If there was a way to use word-wrapped code blocks we should do that. Otherwise we could consider using indented and maybe italicized or codestyle text, which is used elsewhere in the manual for quotes.
This may also be solvable with some custom CSS but that should be avoided if possible.
I'll look around and see if there's any better way here.
There was a problem hiding this comment.
What about manually inserting a line break?
There was a problem hiding this comment.
That might work, if there is a natural sentence break. We definitely should not manually wrap these to 80 characters or similiar.
In the engine these strings are on a single line, though, and you have to scroll horizontally there too. So maybe it's fine as-is.
| ------------------------ | ||
| In GDScript, instance variables can be set by declaring them outside of a method. Additionally, they can be given a default value using ``=``:: | ||
|
|
||
| extends Area2D |
There was a problem hiding this comment.
| extends Area2D |
I'm not sure we need the extends Area2D here for the example to work, but maybe it increases clarity to keep in?
There was a problem hiding this comment.
Overall, I agree - it's not used in this example script, so it could be removed if need be. The main reason I kept it is so that it implies we're looking at an entire script, rather than just an isolated snippet.
In the later code blocks, in addition to serving that purpose, it also provides a practical reason for attempting to assign a node type variable (saving the CollisionShape2D of an Area2D seems like a likely thing to want to do).
There was a problem hiding this comment.
Let's favor keeping it in, then. I'd like opinions from other reviewers on this one.
We may want to add an empty line between extends and var, to follow the style guide.
tutorials/scripting/gdscript/warnings/GET_NODE_DEFAULT_WITHOUT_ONREADY.rst
Outdated
Show resolved
Hide resolved
tutorials/scripting/gdscript/warnings/GET_NODE_DEFAULT_WITHOUT_ONREADY.rst
Outdated
Show resolved
Hide resolved
tutorials/scripting/gdscript/warnings/GET_NODE_DEFAULT_WITHOUT_ONREADY.rst
Show resolved
Hide resolved
tutorials/scripting/gdscript/warnings/GET_NODE_DEFAULT_WITHOUT_ONREADY.rst
Show resolved
Hide resolved
tutorials/scripting/gdscript/warnings/GET_NODE_DEFAULT_WITHOUT_ONREADY.rst
Outdated
Show resolved
Hide resolved
|
These warnings are also documented (rather briefly) in the project settings, for example https://docs.godotengine.org/en/latest/classes/class_projectsettings.html#class-projectsettings-property-debug-gdscript-warnings-get-node-default-without-onready. We may want to link from each new page to the projectsettings class reference too. Though unlike a lot of descriptions in the class reference projectsettings, these are very short and don't add all that much value |
Co-authored-by: tetrapod <[email protected]>
|
I like the idea of providing links to the class reference. Additionally, we could use the descriptions already written in the class reference as the short one-line descriptions for the new pages, before further sections go into more detail and provide examples. |
|
Using the existing short descriptions as placeholders makes sense; but let's wait to validate the overall structure before changing all the instances |
|
I haven't added the short descriptions as placeholders, but I did paste in the warning message strings for each warning 😅 This way, if someone searches the warning message they receive, they'd hopefully find the associated page in the docs. A big issue with this is that messages that have details filled in at runtime are tricky to include. For now, I've just kept them verbatim from the source code with |
|
It would also be nice to mention the default level (Ignore, Warn, Error), in which cases it is recommended or not recommended to enable/disable/suppress the warning. For the warning message, we could use human-readable placeholders instead of Can we use snake_case or kebab-case for file names, while UPPER_CASE for warning names and TOC items? |
For RST filenames, snake case is currently idiomatic |
|
I can certainly change the filenames to use snake case, and replace the instances of |
|
|
||
| This is because ``assert(false)`` calls are often used in development to forcibly halt program execution and avoid strange errors later on. | ||
|
|
||
| See `issue #58087 <https://github.com/godotengine/godot/issues/58087>`_ for more information. |
There was a problem hiding this comment.
| See `issue #58087 <https://github.com/godotengine/godot/issues/58087>`_ for more information. | |
| See `GH-58087 <https://github.com/godotengine/godot/issues/58087>`_ for more information. |
Usually it's styled like this.
Linking to the original issue/PR from the docs seems like an antipattern though, usually we would rewrite the relevant content of the issue/PR into a form suitable for the manual. I'm not wholly against linking though
There was a problem hiding this comment.
I like the idea of linking the issue/PR, similar to how one might paraphrase a primary source in an essay and then provide a citation to the primary source itself, so the reader can look at it themselves if they so wish. But the reasoning provided in the issue might be general enough that it doesn't "need" a citation, and it could also make it unclear when it's appropriate to link to GitHub, resulting in the docs becoming more cluttered.
| @@ -10,27 +10,39 @@ The warning message is: | |||
| When this warning occurs | |||
| ------------------------ | |||
|
|
|||
| The :ref:`assert() <class_@GDScript_method_assert>` keyword can be used to ensure that a given condition is met before allowing code execution to continue. If the first argument passed to it evaluates to ``true``, the rest of the function will run as expected; if it is ``false``, then the project will stop. | |||
| The :ref:`assert() <class_@GDScript_method_assert>` keyword can be used to ensure that a given condition is met before allowing code execution to continue. If the first argument passed to it is truthy, the rest of the function will run as expected; if it is falsy, then the project will stop. | |||
There was a problem hiding this comment.
It would be nice if we defined truthy/falsy somewhere else in the GDScript docs that we could link to, see also #4408
There was a problem hiding this comment.
I agree. In general I'm nervous about even using the terms "truthy" and "falsy" right now, because neither of them currently appear in the docs at all from what I can see (searching either of them on the online docs turns up no results).
Co-authored-by: tetrapod <[email protected]>
Closes #10627.
This begins the implementation of pages describing each warning that can be given by GDScript. A page with all of the warnings can be found at
/tutorials/scripting/gdscript/warnings/index.htmlor Manual > Scripting > GDScript > GDScript warnings. The only page actually implemented so far is forGET_NODE_DEFAULT_WITHOUT_ONREADY; it describes what causes the error, as well as approaches to fixing it.I'd especially appreciate feedback on the structure of the warning page prototype, so that I can incorporate it into writing pages for other warnings. Perhaps once there are short stub descriptions added for all of the warnings, the PR could be merged and then others could take up the task of filling out more pages.