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

Overhaul Object's Documentation #67880

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Oct 25, 2022

This PR is touchy

Closes godotengine/godot-docs#4894
Closes godotengine/godot-docs#5139
Closes godotengine/godot-docs#3827 (???)
... and maybe some more?

Continuation of #67072, #67100, #67208, #67718...

Here a big list of changes which, unlike the previous PRs, is a lot more detailed. Although, I have to admit, I got tired of writing it due to repetition.
Mainly, it's meant to represent my line of reasoning behind every change. Perhaps, someone reading this may take it as a future point of reference. Not just that, but some information deemed important by the maintainers could have been lost during the rewrite. Criticism is encouraged.

General

  • Made use of [param] and several other strong references more often.
  • Corrected typos.
  • Stripped awkward, needlessly long sentences.
  • Simplified wording where it felt necessary. For example:
    • "Contrarily to" -> "Unlike";
    • "taken into account" -> "included"_.
  • Changed wording where technical terms (also used in the rest of the documentation) are more appropriate:
    • "can take" -> "supports";
  • Used synonyms when words could be mistaken for another concept of Godot's API. For example:
    • "parent classes" -> "inherited classes";
    • "plural object" -> "message's subject"
  • Added way more examples.
  • The spacing of comments in the examples has been adjusted to improve readability.
  • Tried to refer to "signal" as "signal name" sometimes.
    • This is because Signal can be a Variant type in Godot 4, but the API may still need to refer to them by name.

Methods

  • The leading description has been completely overhauled.
    • "a built-in type"
      What? Node type? Class type?
      Unclear. It's Variant type.
    • "Objects do not manage memory.",
      Then, literally the next paragraph began with "Some classes that extend Object add memory management".
    • "Objects export properties, which are mainly useful for storage and editing, but not really so much in programming."
      W-what!?
    • The in operator checks for properties, methods, and signals (try this I dare you).
    • Introduced basic concept of inheritance.
    • Introduced Scripts.
    • Introduced Notifications.
    • Introduced Metadata.
  • Reworked _get_property_list.
    • Added important notes.
  • Reworded _init.
    • Specified to be the script, not the Object.
  • Reworded can_translate_messages.
    • "if the object can translate strings".
      Eh? What strings?
  • Reworded connect.
    • Detail returned Error constant.
    • It's better to reword it for now....
      The description of this method is... so needlessly long, verbose, and incredibly horrifying to look at! Despite this, ironically, this isn't even the recommended way to connect signals anymore. This may need to be seen in another PR, it's just... ridiculous. It even explains... emit_signal? Why not putting the explanation there, then!? Aaah...
    • "One can pass a binds Array to Object.connect()" this is just plainly false in Godot 4.
  • Reworked emit_signal.
    • Detail returned error Error constant.
  • Reworked get_incoming_connections.
    • It was entirely wrong and not updated from 3.x.
  • Reworked get_indexed and set_indexed.
    • Add missing note for C# about Built-in pascal_case properties.
  • Reworked get_method_list.
  • Reworked get_property_list.
    • Added more detail to the returned Dictionary Array.
  • Reworked get_signal_list.
    • Added more detail to the returned Dictionary Array
      (Which is incredibly strange by the way).
  • Reworded notify_property_list_changed.
    • "Notify the editor that the property list has changed by emitting the property_list_changed signal".
      These lines have been swapped, because all this method does is emitting the signal.
  • Reworded set_script.
    • "Assign script" -> "Attach script".
      Although both terms are correct, "attach" matches the more familiar verb used in the Scene Tree Editor.
    • "Each object can have a single script assigned to it".
      Very much sounded like a leftover from when Godot used to support multiple scripts.
  • Reworded to_string.
    • "defaults to "ClassName:RID""
      This was outdated.

Signals

  • Add missing description to property_list_changed.
  • Recommend deferring script_changed on some cases.
    • This is because when the signal is emitted, the new script is not yet initialized.

Constants

  • Specified that NOTIFICATION_POSTINITIALIZE is pretty much used internally.
    • You can't receive it from Script, because normally it is sent even before a Script's _init() is even called...
  • Made all CONNECT constant descriptions consistent.
    • They all begin like "[adjective] connections do this thing". I am not sure they are the best way to word them, but it is consistent.

TODO:

  • Finish rewriting leading description.
  • Possibly adjust connect to reduce... overwhelming-ness. Barely... May not be doable for now.
  • Document _set and _get's very interesting use cases (when it is actually called). Can't be done as I wished. It almost feels like the behavior is bugged.
  • Refine get_property_list and _get_property_list.

I recommend taking a look at the documentation by building from this PR, to read the documentation from the Editor itself. Feedback is welcome.

doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch 2 times, most recently from c8e779a to 5ce971c Compare October 26, 2022 10:15
@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 26, 2022

Still unfinished. Introduced metadata in the leading description. it's a core concept, sounds well deserved. Refined _get_property_list() with a really nice example, I find.

@Mickeon Mickeon force-pushed the doc-peeves-objection branch from 5ce971c to 0a2f8f3 Compare October 26, 2022 12:28
@Mickeon Mickeon requested a review from timothyqiu October 26, 2022 12:31
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch from 0a2f8f3 to 2d2fae1 Compare October 26, 2022 22:07
@Mickeon Mickeon requested a review from timothyqiu October 26, 2022 22:09
@Mickeon Mickeon force-pushed the doc-peeves-objection branch from 2d2fae1 to a71178e Compare October 26, 2022 22:15
@Mickeon Mickeon marked this pull request as ready for review October 26, 2022 22:15
@Mickeon Mickeon requested a review from a team as a code owner October 26, 2022 22:15
@Mickeon Mickeon force-pushed the doc-peeves-objection branch from a71178e to 16a5f0e Compare October 26, 2022 23:05
@Mickeon Mickeon changed the title Rewrite all of Object's Documentation Overhaul Object's Documentation Oct 26, 2022
@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 26, 2022

The leading description is in a pretty good state now. Ready for review.

doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch 3 times, most recently from 703de3a to af04134 Compare October 27, 2022 11:20
@Mickeon
Copy link
Contributor Author

Mickeon commented Oct 27, 2022

Another one bites the dust.

doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch 2 times, most recently from 060f539 to d9e50c8 Compare November 21, 2022 01:02
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 21, 2022

If we want to consider my comment as a patch note, I modified the example of _get_property_list() as it may or may not have been problematic, and added examples and further explanation for _get() and _set()

@Mickeon Mickeon force-pushed the doc-peeves-objection branch 2 times, most recently from 1391f52 to bed2eea Compare November 21, 2022 01:22
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch from bed2eea to 3828c03 Compare November 23, 2022 15:51
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 23, 2022

Revision to leading description after @YuriSizov 's comments. The suggestions have not been exactly accepted word-by-word so it may be worth a second look:

  • Changed short description
  • Moved "In GDScript, you can also check if a given property, method, or signal name exists in an object with the [code]in[/code] operator:" to its own paragraph;
  • Removed C#'s "lack of in operator" example;
  • Modified invalid reference note.

@Mickeon Mickeon force-pushed the doc-peeves-objection branch from 3828c03 to d3b25f9 Compare November 23, 2022 16:10
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I thought I'd approve it as is, but I think there are still a couple of changes that are worth to be made before merging. Not everything is severe, so I tried to note the ones where I just nitpick with comments. But there are also a couple of typos 🙂

doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
doc/classes/Object.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-objection branch from d3b25f9 to bee1b52 Compare November 23, 2022 17:13
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 23, 2022

Didn't go with the suggestions 1:1, but I'm particularly glad to have touched upon the _init one because it was explained very awkwardly for being such an important part of the engine.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I'm still not a fan of "If any other means are used", but I won't die on that hill. Let's merge!

@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 23, 2022

"If any other... method?" Screw it, that actually makes more sense.

@YuriSizov
Copy link
Contributor

"If any other... method?"

Well, I've given an example of how I'd rephrase it, but let's not. It's been built enough times already. I'm pressing the merge button!

@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 23, 2022

OKAY FINE MEGA MERGE IT IS.

@YuriSizov YuriSizov merged commit 5923df9 into godotengine:master Nov 23, 2022
@YuriSizov
Copy link
Contributor

Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants