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

C#: Finish documentation of GodotSharp public API #79559

Closed
wants to merge 1 commit into from

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Jul 16, 2023

This PR is exclusively aimed at documentation, so absolutely no code changes were made. The sole exception is GodotSharp.csproj with the removal of <NoWarn>CS1591</NoWarn>, as it now has an accompanying docstring for every single public API.

It's entirely possible that some parts of the documentation aren't fully representative of their implementation, as the catch-22 of this scenario was a lack of documentation to know what to document. In addition, many sections were implemented through automation; while I tried my best to double-check for any potential discrepancies, it's entirely possible some still slipped through the cracks. Nonetheless, I'm largely confident in what's in place now & making adjustments at this stage will be significantly easier.

@Repiteo Repiteo requested a review from a team as a code owner July 16, 2023 19:12
@RedworkDE RedworkDE added this to the 4.x milestone Jul 16, 2023
@Repiteo Repiteo force-pushed the dotnet-xml-comments branch 2 times, most recently from fc72971 to 44be0a1 Compare July 17, 2023 16:30
@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 17, 2023

Implemented a few new fixes, most to the initial commits but a handful to existing docstrings. These include:

Every change remains strictly documentation

@Repiteo Repiteo force-pushed the dotnet-xml-comments branch 3 times, most recently from 31d5597 to 149a4eb Compare July 18, 2023 15:43
@RedworkDE
Copy link
Member

Implemented a few new fixes, most to the initial commits but a handful to existing docstrings. These include:

I would suggest separating these fixes into a new PR, as some of the new documentation will probably need a lot of discussion and some of these bits (at least everything in the Bridge namespace) are not even really supposed to used, except by the engine.

@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 21, 2023

I would suggest separating these fixes into a new PR, as some of the new documentation will probably need a lot of discussion

Done! Those bits were moved to #79748

and some of these bits (at least everything in the Bridge namespace) are not even really supposed to used, except by the engine.

Strange then that CS1591 would consider all of those "public". In either case, assuming the Nowarn will still be removed at some point, would it be better to instead have the GodotSharp .editorconfig handle more specific cases? eg:

[**/Bridge/**.cs]
# Missing XML comment for publicly visible type or member
dotnet_diagnostic.CS1591.severity = none

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.

Thanks for contributing to the .NET documentation. There's a lot here so this is only a partial review.

A lot of these are not very useful since they just state the obvious. Just adding documentation to get rid of the warning is not good enough IMO. I think it's better to keep an API undocumented than document it poorly, because there's no warning for poor documentation.

You said you automated some of these. That's a bit of a red flag. I would encourage you to remove the documentation that you are not sure of. There's no need to document the entire API all at once, how about changing this PR to document only some things? And then we can leave the rest for a future PR? For example, I would recommend to leave out the documentation for Bridge and NativeInterop since those are internal APIs meant to be used by the engine and it's likely difficult for contributors to document them.

Also, it's important to me that the documentation is consistent. This means we should use the same phrasing/wording for similar APIs. For example, take a look at the Equals method in any of the C# structs (i.e. Vector2) and you'll see that we usually use the same style, but the documentation for NodePath.Equals is very different from that. We also try to keep the documentation somewhat consistent with Core.

[StructLayout(LayoutKind.Sequential)]
public unsafe struct ManagedCallbacks
{
// @formatter:off
/// <summary>
/// Delegate for <see cref="SignalAwaiter.SignalCallback"/>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these descriptions are very useful, perhaps we should just disable the warning for these delegates?

@@ -4,46 +4,155 @@

namespace Godot.Bridge
{
/// <summary>
/// Collection of managed callbacks to handle a dotnet hot-reload environment.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "handle a dotnet hot-reload environment"? This struct just contains function pointers that are retrieved by the engine so it can call the C# methods from C++.

#pragma warning disable CS1591 // Disable warning: "Missing XML comment for publicly visible type or member"
///<value><c>(0.94, 0.97, 1, 1)</c></value>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these documentations are useful and would be very difficult to maintain. I guess it's unlikely these values will change, but it has happened before so it's not impossible. I think it'd be better to disable the warning here, like we were doing before with the #pragma.

/// <summary>
/// Returns the exponential of this quaternion.
/// </summary>
/// <returns>The exponential.</returns>
public readonly Quaternion Exp()
Copy link
Member

Choose a reason for hiding this comment

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

I left these Quaternion methods (exp, get_angle, get_axis and log) undocumented because they are undocumented in Core as well. They should be documented in Core and the C# documentation should be in sync with that.

<method name="exp" qualifiers="const">
<return type="Quaternion" />
<description>
</description>
</method>

If you want to document these methods I would recommend to open a new PR that focuses entirely on these methods and documents them for Core too.

@@ -157,7 +157,7 @@ public unsafe void CallDeferred(params Variant[] args)
/// <list type="number">
/// <item>
/// <term>delegateObj</term>
/// <description>The given <paramref name="delegate"/>, upcast to <see cref="object"/>.</description>
/// <description>The given <paramref name="delegate"/>, upcast to <see langword="object"/>.</description>
Copy link
Member

Choose a reason for hiding this comment

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

You forgot to undo this change. See #79748 (comment).

@Repiteo
Copy link
Contributor Author

Repiteo commented Jul 22, 2023

Very, very good points raised all around. In particular, I didn't realize just how synced a lot of the existing documentation was to existing implementations and, more importantly, the core itself. That, alongside the realization that many of these areas are meant only for internal use, made a good chunk of this PR a misguided effort

Closing in favor of multiple, more focused PRs at some later point. I'd rather have fresh starts than try to retrofit something of this scope

@Repiteo Repiteo closed this Jul 22, 2023
@Repiteo Repiteo deleted the dotnet-xml-comments branch October 28, 2023 23:53
@AThousandShips AThousandShips removed this from the 4.x milestone Oct 29, 2023
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