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 String Documentation #68838

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Nov 18, 2022

Adding this one to the collection of #67072, #67100, #67208, #67718,... #67880 and #68560... and #68649...

Related to #68217.

This PR modifies the String documentation extensively in an attempt to improve its quality and make old and new users more at ease while exploring the enormous bloat of a Variant that String is.
Because this is core content that has probably existed since the beginning of time, a review is particularly more appreciated than usual.

Here a big list of changes. 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.

Do note that some exceptions or unexplained changes may be present all around. These are typically done to improve the flow of the sentence.

General

  • Added more detail to the leading description;

  • Made the wording match how other classes are documented a lot more closely;

  • Made use of [param] and several other strong references more often;

  • Linked several related methods to each other.

    • String's documentation is huge, so this feels even more necessary than usual.
  • Included several more links to external websites;

  • Stripped awkward, needlessly long sentences;

  • Changed words where it felt necessary, where they could be mistaken for another concept of Godot's API, where technical terms are more appropriate. For example:

    • "more performant" -> "faster";
    • "a string" -> "the string"/"this string".
      -You're not passing a generic String parameter, several method refer to this very String you're calling it from.
    • "the string providing this method" -> "the string calling this method.";
    • "an array of bytes" -> "[PackedByteArray]";
    • "contains" -> ... (something else);
      • The word is a method of String, and is not an exact match. "A string containing a valid number" is not the same as "A string representing a valid number".
    • _"a number" -> _"a string representing a number".
  • Modified examples extensively, with the main goal to improve readability and variety:

    • Removed quotes around results when they could be misunderstood as strings (# Prints "5" -> # Prints 5);
    • Improved consistency all around;
    • Avoided big walls of print();
    • Added extra padding before comments.
  • Removed quotes when referring to single characters;

  • Removed "case-sensitive" in original method (match, etc.);

  • Highlighted "case-insensitive" more in method variations (matchn, etc.).

Methods

bigrams

  • Modified example.
    • It's to briefly showcase that it's not just letters, but all characters of a string.

bin_to_int

  • "and they can also start with a [code]-[/code] before the optional prefix"
    What for?
  • Modified example to showcase negative numbers.

capitalize

  • "Tables of these characters can be found in various locations"
    Describing it like ASCII tables are quest items.
  • Added examples.

casecmp_to

nocasecmp_to

naturalnocasecmp_to

  • Removed verbose "Behavior with empty strings" paragraph.
    • It has been merged with the first much more smoothly.
  • Improve summary of naturalnocasecmp_to's specialty.

contains

  • Added examples.
  • Moved introduction to in operator with String from find() to here.

find

  • Added substantial examples.
  • Moved introduction to in operator with String from here to contains().

get_base_dir

get_basename

Added examples.

get_extension

Modified examples to improve variation.

get_slice_count

Changed description to imply that no actual splitting (processing an array of substring slices) is executed.

hash

Reduced verbosity of extra note considerably.

indent

  • "The prefix can be any string so it can also be used to comment out strings with e.g. "# "."
    O-o... ok...?
  • Merged unnecessary paragraph to the top.

is_relative_path

  • Reworded entirely, to reduced verbosity considerably.
  • Mentioned reason as to why Node is even mentioned in the first place.

is_subsequence_of

is_subsequence_ofn

  • "Returns true if this string is a subsequence of the given string."
    Ok, cool- Err, I mean, what!? What is a subsequence!?

is_valid_filename

Merged paragraph to the first.

is_valid_float

is_valid_hex_number

is_valid_html_color

is_valid_int

  • Detailed the validation requirements extensively.
  • Modified examples to improve variation.

is_valid_ip_address

Added IPv6 address example.

join

Modified and expanded example.

json_escape

Detail what to use to unescape the string, if necessary.

left

right

Changed example string for better demostration.

length

Detail return value for empty strings.

md5_buffer

md5_text

sha1_buffer

sha1_text

sha256_buffer

sha256_text

Add external links for MD5, SHA-1 and SHA-256.

num_int64

num_scientific

num_uint64

Added missing documentation.

path_join

Reworded entirely to mitigate the general confusion of what this method actually does (it's really, really simple, guys).

repeat

Detailed what happens when parameter is lower or equal to 0.

rsplit

split

  • Reduced verbosity considerably;
  • Fixed wrong example;
  • Expanded example slightly.

simplify_path

Added much more detail and example.

split_floats

Added more examples.

strip_edges

Described left and right parameters in separate paragraph.

to_ascii_buffer

Massively reduced verbosity.

to_float

to_int

Add example result for invalid string.

uri_decode

uri_encode

Modified examples to reference the online documentation.

validate_node_name

Added missing Scene Unique Node symbol (I wish I could forget about them, too but they're here to stay).


Feedback is very, very welcome.

@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch 2 times, most recently from 1192c0a to 48bf9e9 Compare November 18, 2022 16:34
@Chaosus Chaosus added this to the 4.0 milestone Nov 18, 2022
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 48bf9e9 to 9e49e3d Compare November 18, 2022 21:01
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 18, 2022

You may take a second look, although this is still a draft.

doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Show resolved Hide resolved
doc/classes/String.xml Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 9e49e3d to 34c367e Compare November 19, 2022 11:53
@Mickeon Mickeon requested a review from MewPurPur November 19, 2022 14:07
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 34c367e to 42ad84a Compare November 19, 2022 14:08
doc/classes/String.xml Outdated Show resolved Hide resolved
[/codeblock]
</description>
</method>
<method name="is_valid_int" qualifiers="const">
<return type="bool" />
<description>
Returns [code]true[/code] if this string contains a valid integer.
Returns [code]true[/code] if this string represents a valid [int]. A valid integer only contains digits, and may be prefixed with a positive ([code]+[/code]) or negative ([code]-[/code]) sign. See also [method to_int].
Copy link
Contributor

@MewPurPur MewPurPur Nov 19, 2022

Choose a reason for hiding this comment

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

No need to refer to the int class IMO. A valid integer is not a valid [int] - it doesn't support multiple unary operators or separators. This is for GDScript, anyway... It also supports arbitrarily big numbers, not just 64-bit.

Or we could perhaps add a note about it.

Same for is_valid_float(), there are even more inconsistencies there.

Look what I had to do when I wanted to catch up GDScript's highlighter to accept all valid floats/ints and halt otherwise 😂

image

Copy link
Contributor Author

@Mickeon Mickeon Nov 19, 2022

Choose a reason for hiding this comment

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

Unsure. The GDScript compiler is one thing and the int type is another. Many different languages may define their own special way to write an integer, but at its core, in Godot an int is an integer and a float is a floating-point value. I may or may not make the change.

Copy link
Contributor

@MewPurPur MewPurPur Nov 19, 2022

Choose a reason for hiding this comment

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

Yeah I was GDScript-specific here - anyway, since these methods have their own generic rules for what is a valid int/float, I'd say it's better to just state those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Come to think of it, one method was renamed from 3.x. It used to be called is_valid_integer. The current name is a bit of a misnomer because it could really just go above the 64-bit limit if someone wanted to use it for that purpose... So it's not just about the Variant Type called int, it's any kind of integer.

doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch 3 times, most recently from b1e06b7 to 1511d3d Compare November 19, 2022 22:05
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 1511d3d to 1ce2bd6 Compare November 19, 2022 23:07
@Mickeon Mickeon requested a review from bruvzg November 19, 2022 23:07
doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 1ce2bd6 to bf404e9 Compare November 19, 2022 23:20
@Mickeon Mickeon changed the title Tweak String Documentation Overhaul String Documentation Nov 19, 2022
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch 2 times, most recently from 1d3fc5c to 3f76a27 Compare November 19, 2022 23:51
@Mickeon Mickeon marked this pull request as ready for review November 19, 2022 23:51
@Mickeon Mickeon requested a review from a team as a code owner November 19, 2022 23:51
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 19, 2022

Settles it. At this point it is pretty much ready for review.

Copy link
Contributor

@MewPurPur MewPurPur left a comment

Choose a reason for hiding this comment

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

Last one fr this time

doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Show resolved Hide resolved
[/codeblock]
</description>
</method>
<method name="is_valid_int" qualifiers="const">
<return type="bool" />
<description>
Returns [code]true[/code] if this string contains a valid integer.
Returns [code]true[/code] if this string represents a valid integer. A valid integer only contains digits, and may be prefixed with a positive ([code]+[/code]) or negative ([code]-[/code]) sign. See also [method to_int].
Copy link
Contributor

Choose a reason for hiding this comment

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

comma before "and" not necessary here.

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'm putting it here this time to nicely split the requirements for an integer.

doc/classes/String.xml Outdated Show resolved Hide resolved
@@ -124,7 +149,7 @@
<param index="1" name="from" type="int" default="0" />
<param index="2" name="to" type="int" default="0" />
<description>
Returns the number of occurrences of substring [param what] between [param from] and [param to] positions. If [param from] and [param to] equals 0 the whole string will be used. If only [param to] equals 0 the remained substring will be used.
Returns the number of occurrences of the substring [param what] between [param from] and [param to] positions. If [param to] is 0, the search continues until the end of the string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Numbers not surrounded by code pt. 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fine, actually, do not worry about surrounding numbers in code too much. Doing it here ends up probably filling the description with more codeblocks and padding than necessary. Will keep open for further evaluation.

doc/classes/String.xml Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from b3e2a00 to 637e386 Compare November 20, 2022 20:43
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 637e386 to c5f2272 Compare November 22, 2022 12:32
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 22, 2022

Updated this PR to add a boolean context note in the leading description. This note is present in several other Variant types, so it only makes sense to include it here as well, unless there are some objections to it.

@MewPurPur
Copy link
Contributor

MewPurPur commented Dec 2, 2022

While looking at the String class today, I noticed that some of the operator descriptions say "left string" and some say "this string". The latter felt confusing.

My thought is, we should improve the way operators are presented in the docs overall, and alongside that visit all operator documentation to be explicit about "left" and "right".

I also saw "both" being misused - it can't be used like "if both strings have something in common" - must be "if the two strings have something in common".

@Mickeon
Copy link
Contributor Author

Mickeon commented Dec 2, 2022

My thought is, we should improve the way operators are presented in the docs overall, and alongside that visit all operator documentation to be explicit about "left" and "right".

Not sure. All operators act from a base and right is the input Variant. Some operators benefit from being explained one way, while others, not so much.

Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks everyone for the reviews!

doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 14cc3a3 to 9e77dce Compare December 5, 2022 17:27
@Mickeon Mickeon requested a review from raulsntos December 5, 2022 17:31
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 9e77dce to 6164497 Compare December 5, 2022 17:31
doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 6164497 to 3f2d0cd Compare December 5, 2022 18:25
@Mickeon Mickeon requested a review from raulsntos December 5, 2022 18:26
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

LGTM minor a nit pick, but it's probably fine to leave as is.

doc/classes/String.xml Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the doc-peeves-3-strings-&-3-sticks branch from 3f2d0cd to 3b71d85 Compare December 5, 2022 19:33
@akien-mga akien-mga merged commit ae37045 into godotengine:master Dec 6, 2022
@akien-mga
Copy link
Member

Thanks!

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