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

Make RST docs consistent with Editor Help docs #80690

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Aug 16, 2023

Built-in documentation uses a syntax similar to GDScript, while the online documentation uses C-style.

1. Use method_name(param1: Type1, param2: Type2 = default) signature
instead of method_name ( Type1 param1, Type2 param2=default ):

I think parentheses without spaces would look better with a monospaced font.

2. Add tooltip for void type:

3. Use Array[Type] instead of Type[]:

4. Use BitField[Type] instead of BitField<Type>:

5. Highlight pointer types (in extension documentation):

Maybe some other way to highlight is needed. For example bold font, but I'm worried about escaping * and **.

@dalexeev dalexeev added this to the 4.2 milestone Aug 16, 2023
@dalexeev dalexeev requested a review from a team as a code owner August 16, 2023 15:34
@dalexeev
Copy link
Member Author

See also #80765 (comment). I'm not sure if we should show the full signature, or if we should hide the types and qualifiers.


@YuriSizov
Copy link
Contributor

I'm conflicted about the method signature change because it's still neither GDScript nor C-like. It's still a mix of both, and I don't really understands who does it serve in this form (current form included, not just the changes from this PR).

Specifically, the return type in GDScript would be written in a different way, obviously. But to my taste it makes sense to put it in front of the method. But that's inconsistent with GDScript, though we are not fixing this inconsistency. So making other changes like that seems arbitrary to me. Not that I don't support it, just that I can't define a line other than personal tastes and weak consensus between maintainers.

Then there is 5, which adds a pointer symbol to an otherwise GDScript-like format. This is unusable in GDScript, but it's still written similar to it. This makes me think that C-like is a better common denominator for a language-agnostic documentation.


2, 3, and 4 are good. For setters and getters I'd keep the full signature.

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Sep 27, 2023
@Mickeon
Copy link
Contributor

Mickeon commented Jan 26, 2024

How do we feel about this? I honestly like all of the changes except the method signature for the same reasons brought up above. I feel like the rest just "read" better for most people.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 18, 2024

My question from 3 weeks ago still stands. I think anything but the method signature is an objectively good thing. Would be good to rebase.

@dalexeev dalexeev force-pushed the make-rst-docs-consistent-with-editor-help branch from 4d43a0e to 8ed19d1 Compare February 19, 2024 17:19
@dalexeev
Copy link
Member Author

I understand why point 5 is controversial, but I think that points 3 and 4 make no sense without 1, especially since this is how it's done in EditorHelp. We are making the signature more similar to GDScript, the only difference is that the return type is before the method name, because of the table. See also godotengine/godot-proposals#7607.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Actually, You know what? Fine. It is consistent with the built-in docs. I was thinking about it for a bit. We shouldn't be afraid to change this. It just plainly looks better. Worst case scenario, it is reverted.

In a perfect world there would be a toggle between GDScript and C# definitions but this is no perfect world.

@@ -1638,6 +1652,7 @@ def make_footer() -> str:
f".. |static| replace:: :abbr:`static ({static_msg})`\n"
f".. |operator| replace:: :abbr:`operator ({operator_msg})`\n"
f".. |bitfield| replace:: :abbr:`BitField ({bitfield_msg})`\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes you really think if BitField should have been called BitFlags (on the outside) all along. "field" for these kinds of properties is rarely ever used. Usually they're referred to as "masks", or "flags".
Question for another time, of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the name of the core class. If we ever add such a type to GDScript, it would probably make sense to name it BitField[T] for consistency.

@Mickeon
Copy link
Contributor

Mickeon commented Feb 19, 2024

I must note, however, I don't think this closes the original proposal. OP still desires the the returned value to be on the right as per GDScript standard.
While that can be done, it needs to first be evaluated, as it would be very alienating for most users. Second, it needs to be well-thought out because with the current way built-in docs display method attributes, doing it won't look pretty.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga merged commit da05766 into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the make-rst-docs-consistent-with-editor-help branch February 20, 2024 10:14
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