godot-docs
godot-docs copied to clipboard
Add GDScript warning pages
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.
This initial PR adds pages for five warnings commonly encountered by beginners:
- GET_NODE_DEFAULT_WITHOUT_ONREADY
- INTEGER_DIVISION
- RETURN_VALUE_DISCARDED
- SHADOWED_VARIABLE
- UNASSIGNED_VARIABLE
- UNUSED_VARIABLE
If this PR is successful, small batches of additional warning pages can be added in subsequent PRs, so that they can be reviewed and verified before being published.
Each page has the following sections:
- An example of the warning message that appears in Godot, so that it can be easily found if someone pastes the message into a web search
- An explanation of what causes the warning to appear, including sample code
- Instructions on how to fix or suppress the warning
The target audience for the pages is relative newcomers to GDScript and/or programming in general; people who may be using code from tutorials without fully understanding it, and thus could be confused by the warning messages Godot provides. As such, the explanations aim to be extra beginner-friendly.
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
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 %s where text is filled in.
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 %s.
Can we use snake_case or kebab-case for file names, while UPPER_CASE for warning names and TOC items?
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 %s with something more natural.
Adding the default warning level also makes sense to me - I'll try to add these things to my prototype pages soon.
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?
During one of the previous GDScript meetings, we discussed only including a small subset of the GDScript warnings in the initial PR. This way, they'd be easier for others to thoroughly review and approve before merging.
I think it'd make sense, then, to try and choose the most common warnings first, so that they're the most likely to help people. The more obscure warnings can get pages later.
Based on what I've seen from people in online help communities, I think these pages could make for a good initial batch:
- GET_NODE_DEFAULT_WITHOUT_ONREADY
- INTEGER_DIVISION
- RETURN_VALUE_DISCARDED
- SHADOWED_VARIABLE
- UNASSIGNED_VARIABLE
- UNUSED_VARIABLE
Are there any other warnings that would be good to include? On the flip-side, are any of the ones I've listed here unnecessary to start with? Once I have a solid response/consensus on that, I can work on editing the PR's contents to just include those warning pages.
This PR has been updated to only contain warning pages for the six warnings listed above:
- GET_NODE_DEFAULT_WITHOUT_ONREADY
- INTEGER_DIVISION
- RETURN_VALUE_DISCARDED
- SHADOWED_VARIABLE
- UNASSIGNED_VARIABLE
- UNUSED_VARIABLE
We can start adding more pages soon, but hopefully this will make for a good initial batch.
I've tried to refine the GET_NODE_DEFAULT_WITHOUT_ONREADY page, bringing it more in line with the other, later-written pages as well as removing unnecessary background information.