-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add GDScript warning pages #10635
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
base: master
Are you sure you want to change the base?
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`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about manually inserting a line break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is not enough. You could make more obvious that it's an instance variable (basically a property) by declaring a method below that uses it.
It doesn't really matter that much, however. The examples below mitigate the ambiguity
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
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to do this in Variant's own class reference in godotengine/godot#95487 but I haven't had the time to retouch it considerably.
Note that a lot of Variant types describe what happens in "a boolean context" in their respective descriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I may use that wording instead to remain consistent!
fc41f4f
to
a2a6829
Compare
I'm opening this PR for review now. My goal is to have at least a first version complete for all pages before merging, so that users don't go to look up a warning and just find "TODO". There's definitely a lot of work in writing the pages out, so perhaps that is too much to hope for. Regardless, it'll be very nice to have people reading and reviewing what's been written so far. If there's a way to allow other people to contribute pages, that would be great too. I suppose they'd submit a PR on my fork? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is accepted each page should also be linked in the corresponding setting in the class reference.
This is quite a lot to review at once, and yet, as of this swoop, it's quite nice. But oh boy
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The terms "truthy" and "falsy" never appear in the documentation before. We should consider spelling this out, like "is equivalent to true in a boolean context". I only say "boolean context" because a very common note in each Variant type's class reference says as much.
Not that I'm personally against mentioning "truthy" or "falsy", as it's a fairly common thing in programming, but it needs to be explained somewhere, and it's not done anywhere, nor is this the place to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I recall, someone else made the same (or a similar) suggestion a while back, so this is definitely worth revising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this document to use the "boolean context" phrasing - should be pushed soon, once I've made other adjustments.
tutorials/scripting/gdscript/warnings/confusable_identifier.rst
Outdated
Show resolved
Hide resolved
tutorials/scripting/gdscript/warnings/confusable_identifier.rst
Outdated
Show resolved
Hide resolved
var engine_nаme = "Godot" | ||
print(engine_name) | ||
|
||
In this code snippet, the ``print`` statement would fail, because ``engine_name`` is actually not defined. The identifier in the ``print`` statement uses the Latin character "a" (U+0061), while the identifier in the variable declaration above uses the Cyrillic character "а" (U+0430). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oooo, this is nasty, is there any other letter that may be slightly more distinguishable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can double-check later, but I recall going into the code to try and find the "look-alike" table that Godot may have been using to detect these, and was unable to find it (perhaps it was being processed by a third-party library?). The letters being indistinguishable might be helpful here, though. They look identical to each other, yet aren't the same, which is the point of the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this page that visualizes the Unicode confusable characters: https://util.unicode.org/UnicodeJsps/confusables.jsp
For this example, I could replace the Cyrillic character with ɑ (lowercase alpha), like so:
var engine_nɑme = "Godot"
print(engine_name)
There are a few other characters (α, ⍺, 𝐚, 𝑎, etc) that could also be used instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this alternative. It's subtle but at least noticeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should make the intentionally confusable identifier less confusing, even for demonstration in documentation. The point is that some Unicode characters are visually indistinguishable from each other in most fonts. If someone doesn't know this, I think it's better for them to learn it here, rather than expecting characters to be similar but still slightly different.
Or we can clarify it separately, something like Moreover, some characters may have no visual differences at all in most fonts, like Latin "a" and Cyrillic "а".
.
|
||
Assert statement will raise an error because the expression is always false. | ||
|
||
The default warning level for this warning is **Warn**. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be something in Sphinx's restructuredtext that may avoid having to copy and paste this every time. Putting a pin on that.
https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#substitutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I can define a series of substitutions like
.. |default_is_ignore| replace:: The default warning level for this warning is **Ignore**.
.. |default_is_warn| replace:: The default warning level for this warning is **Warn**.
.. |default_is_error| replace:: The default warning level for this warning is **Error**.
and then use them (like |default_is_warn|
) to paste that message in. Automating the ProjectSettings links might take a bit more work, since it involves changing text within the reference link. I would really love to figure out a way to automate this well, since as you pointed out so much of the text is the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken I don't think this would work across multiple pages so there may still be a need to merge everything into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I wouldn't let this narrow issue of substitutions determine if we make one page or multiple pages for these warnings. (FWIW a page for each warning seems natural to me, and there is prededent with the C# diagnostics, but this is a larger decision and it seems like other reviewers are leaning towards a single page)
-
It's likely not worth the effort to automate the class reference links, since that would involve dynamic replacement and IIRC that isn't straightforward with RST substitutions.
-
You can make a substitution accessible from all pages
rst_epilog
, we already use it at least once:
Lines 289 to 298 in 6ae63dd
rst_epilog = """ .. |weblate_widget| image:: https://hosted.weblate.org/widgets/godot-engine/{image_locale}/godot-docs/287x66-white.png :alt: Translation status :target: https://hosted.weblate.org/engage/godot-engine{target_locale}/?utm_source=widget :width: 287 :height: 66 """.format( image_locale="-" if language == "en" else language, target_locale="" if language == "en" else "/" + language, )
However, we don't use that for cases like the upgrading pages or the class reference, where each page redefines the same substitions:
godot-docs/tutorials/migrating/upgrading_to_godot_4.1.rst
Lines 211 to 213 in 6ae63dd
.. |❌| replace:: :abbr:`❌ (This API breaks compatibility.)` | |
.. |✔️| replace:: :abbr:`✔️ (This API does not break compatibility.)` | |
.. |✔️ with compat| replace:: :abbr:`✔️ (This API does not break compatibility. A compatibility method was added.)` |
godot-docs/classes/class_aabb.rst
Lines 770 to 777 in 6ae63dd
.. |virtual| replace:: :abbr:`virtual (This method should typically be overridden by the user to have any effect.)` | |
.. |const| replace:: :abbr:`const (This method has no side effects. It doesn't modify any of the instance's member variables.)` | |
.. |vararg| replace:: :abbr:`vararg (This method accepts any number of arguments after the ones described here.)` | |
.. |constructor| replace:: :abbr:`constructor (This method is used to construct a type.)` | |
.. |static| replace:: :abbr:`static (This method doesn't need an instance to be called, so it can be called directly using the class name.)` | |
.. |operator| replace:: :abbr:`operator (This method describes a valid operator to use with this type as left-hand operand.)` | |
.. |bitfield| replace:: :abbr:`BitField (This value is an integer composed as a bitmask of the following flags.)` | |
.. |void| replace:: :abbr:`void (No return value.)` |
There might be a compile time increase when using rst_epilog, which is why it's not used more. I haven't looked closely at this, though.
There's also .. include
(docs) which could be used to only include the substitutions on the pages that use it (and therefore reduce compile times if using rst_epilog is too heavy). We don't currently use .. include
, but it's usage looks relatively straightforward.
(I still have to do a deeper review of the rest of this later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an RST file and .. include
is how I implemented the test on my end.
warning_page_substitutions.rst
:
.. |default_is_ignore| replace:: The default warning level for this warning is **Ignore**.
.. |default_is_warn| replace:: The default warning level for this warning is **Warn**.
.. |default_is_error| replace:: The default warning level for this warning is **Error**.
assert_always_false.rst
:
ASSERT_ALWAYS_FALSE
=======================
The warning message is:
.. code-block:: none
Assert statement will raise an error because the expression is always false.
|default_is_warn|
To modify it, see :ref:`ProjectSettings.debug/gdscript/warnings/assert_always_false<class_ProjectSettings_property_debug/gdscript/warnings/assert_always_false>`.
.. include:: warning_page_substitutions.rst
With this, we'd only have to edit or translate the sentences once in warning_page_substitutions.rst
and they'd apply everywhere, which is nice. Dynamic substitution would be very helpful for this, but as you said it sounds like it would be quite involved.
tutorials/scripting/gdscript/warnings/standalone_expression.rst
Outdated
Show resolved
Hide resolved
# Will give warning STATIC_CALLED_ON_INSTANCE. | ||
var result = my_math_funcs.subtract_two(5) | ||
|
||
When a function is *static*, it belongs to the class as a whole, not any one specific instance of the class. This can be quite handy for writing functions that you want to access from anywhere, without needing a class instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true, it is handy, but this doesn't explain why this warning exists. Most warnings exist because the code may not be working as intended, but this one is mainly to avoid confusion when reading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good point. I think my intent here was to explain what "static" even meant to someone who perhaps wasn't familiar with the term, but I see here that I neglected to follow up that description with a discussion of the warning itself 😅
I think I might remember there being a section in the GDScript docs that describes static functions/methods; perhaps it could be linked from here. This will certainly need more discussion of why calling a static method on an instance is questionable, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference, the warning was implemented in godotengine/godot#67365 :
The purpose of this warning is to avoid the silent failure resulting from functions which have become static recently
And it is a notable pitfall, too.
I think I might remember there being a section in the GDScript docs that describes static functions/methods
https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#static-variables and https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html#static-functions
When this warning occurs | ||
------------------------ | ||
|
||
This warning may appear when attempting to use a variable that hasn't had a value assigned to it yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth mentioning that in Godot an unassigned variable is essentially null
Co-authored-by: Micky <[email protected]>
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.html
or 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.
Pages to write
Other To-Do