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

Update documentation's "Prints" comments after #47502 #99081

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Nov 11, 2024

See:

To make it brief, print(a) # Prints {value} is a very common way in the class reference to represent a result. What follows it needs to be precisely what is printed in the console. This is not always the case, but it's a pretty consistent rule.

For transparency, my own notes on these kinds of comments is as follows:

  • # Prints {value}: After a print call. (or similar).
    Suitable for beginner topics, where you want the user to see the result upon copying and pasting the code sample. {value} should be formatted exactly how it would be printed to the console. No periods, no quotes.

However, since #47502, whole floating-point numbers are always converted and printed with the trailing .0, making many of these comments inaccurate.

Although this is technically an error, this PR is worth debating about, as it actually makes readability worse in a vast majority of cases.
Either these comments are mostly fine as they are, we make a special exception just for Vectors, or we go ahead and always favour pure accuracy to not confuse the user.

@Mickeon Mickeon added this to the 4.4 milestone Nov 11, 2024
@Mickeon Mickeon requested a review from a team as a code owner November 11, 2024 17:39
@tetrapod00
Copy link
Contributor

I think your original notes are still correct. If you're bothering to add a comment with the expected output, it should match the actual output.

I'm also not sure this decreases readability. In some other contexts (like shaders), adding the explicit .0 to whole number floats is favored, because it makes it clear you're working with a float-based type rather than an int-based type. In this case, I could take it or leave it, but it's not clearly a decrease in readability.

@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 11, 2024

I have to say it somewhat impacts readability when multiple Vector types are involved (see the PR itself). This readability issue is somewhat similar to what this user brought up.
Keep in mind there can be a lot of numbers, dots, and commas packed within these codeblocks, oftentimes in the same line. Also, these numbers within comments have no syntax highlighting.

Problematic examples include:

  • print(polygon) # Prints [(50.0, 50.0), (150.0, 50.0), (150.0, 150.0), (50.0, 150.0)]
  • print(my_basis.x) # Prints (2.0, 0.0, 0.0).
    print(my_basis.y) # Prints (0.0, 4.0, 0.0).
    print(my_basis.z) # Prints (0.0, 0.0, 8.0).
  • var a = AABB(Vector3(4, 4, 4), Vector3(8, 8, 8)).grow(4)
    print(a.position) # Prints (0.0, 0.0, 0.0)
    print(a.size)     # Prints (16.0, 16.0, 16.0)
    
    var b = AABB(Vector3(0, 0, 0), Vector3(8, 4, 2)).grow(2)
    print(b.position) # Prints (-2.0, -2.0, -2.0)
    print(b.size)     # Prints (12.0, 8.0, 6.0)
    

Note that one solution could also be to rework the examples to mitigate this issue, outright.

@tetrapod00
Copy link
Contributor

Of the examples in your last comment, only the polygon example seems unreadable to me, since it's a long line. In the other examples, a single vector print on each line seems fine. But then again I've been mostly working with shader examples where floats always use .0 for clarity.

In general, I think accuracy should be prioritized, since these comments are presumably used to check your work when you copy-paste an example. Some readability loss is acceptable for that, IMO.

I don't feel all that strongly about this, though. Making an exception for vectors, or just overall not caring about matching the exact output, is also a reasonable choice.

doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.xml Outdated Show resolved Hide resolved
doc/classes/AABB.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 documentation-floats-now-print-the-trailing-zero branch from 7435e43 to b9a8142 Compare November 11, 2024 22:37
@Mickeon
Copy link
Contributor Author

Mickeon commented Nov 11, 2024

Addressed the above review across the board.

Comment on lines -15 to +16
print(astar_grid.get_id_path(Vector2i(0, 0), Vector2i(3, 4))) # prints (0, 0), (1, 1), (2, 2), (3, 3), (3, 4)
print(astar_grid.get_point_path(Vector2i(0, 0), Vector2i(3, 4))) # prints (0, 0), (16, 16), (32, 32), (48, 48), (48, 64)
print(astar_grid.get_id_path(Vector2i(0, 0), Vector2i(3, 4))) # Prints [(0, 0), (1, 1), (2, 2), (3, 3), (3, 4)]
print(astar_grid.get_point_path(Vector2i(0, 0), Vector2i(3, 4))) # Prints [(0, 0), (16, 16), (32, 32), (48, 48), (48, 64)]
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 not related to the PR but it looks like an obvious oversight.

@Mickeon Mickeon force-pushed the documentation-floats-now-print-the-trailing-zero branch from b9a8142 to d90f045 Compare November 30, 2024 15:14
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Seems like we're sticking with the trailing zero format for floats, so this style checks out

@Repiteo Repiteo merged commit 61f2001 into godotengine:master Dec 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 11, 2024

Thanks!

@Mickeon Mickeon deleted the documentation-floats-now-print-the-trailing-zero branch December 14, 2024 17:07
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