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 NodePath documentation #80183

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Aug 2, 2023

Currently a draft because it feels like more can still be done. I would love your assistance!

Continuation of similar past efforts with the same goal of refreshing the documentation up a notch.

... Man, this doc has been collecting so much dust!

General

  • Made the wording match how other classes are documented more closely;
  • Changed most self-referential mentions of "NodePath" to "node path";
  • Simplified and clarified the wording marginally. In particular:
    • "prepended" -> "prefixed";
    • "make up" -> "compose";
    • "subnames" -> "property subnames".
    • "gets" -> "returns".
  • Remove mentions of "resource and properties" and similar.
    • All Resources are properties in the context of a NodePath. Not to mention that NodePaths may not necessarily point to a Resource type.
  • Use GDScript's literal syntax for NodePaths (^"") whenever applicable.
  • Modified misleading recurring examples:
    - Path2D/PathFollow2D/Sprite2D:texture:load_path. But load_path isn't even a real thing.
    - There also were a lot of Path2D and PathFollow2D everywhere.
    An outstandingly baffling choice for a class that is literally a path.
  • Overhaul the leading description extensively.
    • Basically all of the topics that need to be talked about have been rearranged to hopefully be easier to digest.
    • Add examples for property paths.

Constructors

NodePath(String)

  • Removed the "absolute VS. relative" explanation.
    • This is not the place to explain it. It's already been done.
    • "Absolute paths are only valid in the global scene tree, not within individual scenes" this is just not true, too?
    • "In a relative path, "." and ".." indicate the current node and its parent." is a bit misleading, too.
      You may use .. in the middle an absolute path just fine if you really, really wanted to

Methods

get_as_property_path

  • Specify it returns a copy of the node path.
  • Make the examples less verbose and more readable.

get_concatenated_names, get_concatenated_subnames

  • Specify that a StringName is returned.
    • Without looking closer, it may be easy to assume an Array of Strings is returned.

get_name, get_subname

  • Modified examples to be slightly less generic and potentially confusing.
  • Describe what happens when the index is out of bounds.

hash

is_absolute

  • Modified description to be considerably less awkward.

is_empty

  • "Returns true if the node path is empty.". Wow, thanks!

Feedback is very, very welcome.

@AThousandShips AThousandShips added this to the 4.x milestone Aug 2, 2023
@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from f672482 to 3def88b Compare August 2, 2023 22:39
@Mickeon Mickeon marked this pull request as ready for review August 2, 2023 22:40
@Mickeon Mickeon requested a review from a team as a code owner August 2, 2023 22:40
@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from 3def88b to d44e523 Compare August 2, 2023 22:41
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
Comment on lines +21 to +22
^"/root/Title" # May point to the main scene's root node named "Title".
^"/root/Global" # May point to an autoloaded node or scene named "Global".
Copy link
Contributor

Choose a reason for hiding this comment

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

Idk about using two examples here. Both could be either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried trimming it to a single example that proposes both possible nodes, but the example does look "uglier" and not as tidy as a result.

doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor

Another note, we could mention that "..", "." also count as names.

@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from d44e523 to 4848c71 Compare August 3, 2023 12:25
@Mickeon
Copy link
Contributor Author

Mickeon commented Aug 3, 2023

Updated PR to address the majority of the review above, along with several tweaks especially to the leading description.

Another note, we could mention that "..", "." also count as names.

Both .. and . are now documented in the leading description. Some examples have changed to use .. as well.

I feel like more can be done (namely, the short description absolutely needs to change), but it currently is in a pretty good state.

@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from 4848c71 to 411b060 Compare September 6, 2023 23:05
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 6, 2023

Very minor rebase after conflict.

@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 30, 2023

Rebased to force the Linux test to start, hopefully without bugging out.

doc/classes/NodePath.xml Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Otherwise the language and style looks good

doc/classes/NodePath.xml Outdated Show resolved Hide resolved
doc/classes/NodePath.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from cd2ec6f to ab727a9 Compare December 30, 2023 15:40
@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 30, 2023

Accepted all of the above suggestions. Thank you.

@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from ab727a9 to 63319b6 Compare January 15, 2024 16:46
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 15, 2024

Rebased after a "conflict" (it didn't really matter, it is overridden entirely).

It would be nice to have this for 4.3 along with the other "polishing" changes.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 15, 2024
@Mickeon Mickeon force-pushed the doc-peeves-noodlespath branch from 63319b6 to 78574dd Compare January 15, 2024 17:00
@akien-mga akien-mga merged commit b9a07ad into godotengine:master Feb 13, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 13, 2024

At long last, thank you. A few of these long-standing ones shall remain.

@Mickeon Mickeon deleted the doc-peeves-noodlespath branch February 13, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants