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#: Document generated members #79239

Merged

Conversation

raulsntos
Copy link
Member

Documents generated members and tries to discourage users from calling/overriding internal methods that only exist to be used by the engine.

@raulsntos raulsntos added this to the 4.x milestone Jul 9, 2023
@raulsntos raulsntos requested a review from a team as a code owner July 9, 2023 12:15
Documents generated members and tries to discourage users from calling/overriding internal methods that only exist to be used by the engine.
@raulsntos raulsntos force-pushed the dotnet/document-generated-members branch from 66cc220 to 12e4aa9 Compare July 9, 2023 12:26
Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

tries to discourage users from calling/overriding internal methods that only exist to be used by the engine.

Should even properly document them in xml docs then or should we just have the warning not to use them and whatever is is necessary to suppress the warnings?

Related: net8 adds an Experimental attribute for things like this: https://github.com/dotnet/designs/blob/main/accepted/2023/preview-apis/preview-apis.md which we could use for these methods once that is released. Or alternatively we could mark these things Obsolete to generate a warning right now when using them.

@raulsntos
Copy link
Member Author

raulsntos commented Jul 10, 2023

net8 adds an Experimental attribute for things like this: dotnet/designs@main/accepted/2023/preview-apis/preview-apis.md which we could use for these methods once that is released

I don't feel like the Experimental attribute fits these APIs, since they are not preview features. I plan to use this attribute for Godot API that we mark is_experimental such as Navigation APIs.

Or alternatively we could mark these things Obsolete to generate a warning right now when using them.

Some of these methods are used by source generators so this would generate warnings for users that don't use them directly. Also, it would generate warnings for us in the GodotSharp project.

Should even properly document them in xml docs then or should we just have the warning not to use them and whatever is is necessary to suppress the warnings?

I think documenting them is useful, because it can be helpful for contributors. I don't think it's that important for users not to call these methods, just letting them know that they are not mean to and hiding them so they don't accidentally stumble upon them is enough.

@RedworkDE
Copy link
Member

With the Obsolete / Experimental I was mostly thinking about future changes´to these methods, eg we explicitly and clearly signal that we may end up changing these methods if future improvements require it. (Unless this is interpretations is completely wrong and they are supposed to be proper members of the public interface, and if it was possible to make them inaccessible they would still be public)

@raulsntos
Copy link
Member Author

My concern is that since some of these methods are used by source generators, those warnings would show up in user projects even if they are not using them directly.

Also, technically we should avoid breaking compat in these methods too, because an external library built against Godot 4.0 could be using them. For example, if a library implements a Node type that the user derives from in their game project, then I think the generated InvokeGodotClassMethod method will throw a MissingMethodException when calling the base method implementation.

And I see what you mean now by using Obsolete / Experimental to mark that these methods may change in the future, but I still feel it's weird to mark the API as obsolete or experimental for this. It's just a matter of semantics for me.

@YuriSizov
Copy link
Contributor

Thanks!

@raulsntos raulsntos deleted the dotnet/document-generated-members branch July 26, 2023 01:53
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.

C# generated code does not include public XML comments or disable 1591
3 participants