godot-docs icon indicating copy to clipboard operation
godot-docs copied to clipboard

Add GDScript warning pages

Open Meorge opened this issue 10 months ago • 11 comments

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.

Meorge avatar Feb 07 '25 03:02 Meorge

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

tetrapod00 avatar Feb 07 '25 04:02 tetrapod00

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.

Meorge avatar Feb 07 '25 20:02 Meorge

Using the existing short descriptions as placeholders makes sense; but let's wait to validate the overall structure before changing all the instances

tetrapod00 avatar Feb 07 '25 21:02 tetrapod00

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.

Meorge avatar Feb 07 '25 21:02 Meorge

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?

dalexeev avatar Feb 12 '25 17:02 dalexeev

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

tetrapod00 avatar Feb 12 '25 17:02 tetrapod00

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.

Meorge avatar Feb 12 '25 20:02 Meorge

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?

Meorge avatar May 13 '25 02:05 Meorge

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.

Meorge avatar Jun 29 '25 23:06 Meorge

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.

Meorge avatar Sep 19 '25 04:09 Meorge

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.

Meorge avatar Sep 25 '25 03:09 Meorge