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

singletons_autoload.rst: add C# example similar to "Global Variable" #8852

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

31
Copy link
Contributor

@31 31 commented Jan 27, 2024

The singleton page leaves C# devs with the clunky GetNode<PlayerVariables>("/root/PlayerVariables"). In the C# discord help channel, adding a public static PlayerVariables Instance { get; private set; } is probably one of the most repeated bits of code. This PR tries adding it to the doc.

@@ -94,6 +94,27 @@ be accessed directly in GDScript, without requiring ``get_node()``:

PlayerVariables.health -= 10

The **Enable** column has no effect in C# code. However, if the singleton is a
C# script, a similar effect can be achieved by adding a static property:
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 think it's awkward enough having a GDScript-only code tab switcher, so I don't think it's great to also add a C#-only one. If there's some way to put it all under C#/GDScript tabs, it seems like that would be nicer? I don't imagine it would be ok to change all the explanation into a code comment, though, and that's the only way I know how to do it.

Another idea is to put this example in the C# docs somewhere (in the "C# language features" page?) and add a note here that links to it. I don't really know how important it is to document C# stuff up front vs. siloing it.

I don't have any strong preferences here, just want to get this in the docs somewhere. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Code can't be translated so I don't think we should move the explanation to code comments.

As for tab switchers with only one tab, we can add code snippets without tabs. See:

But this is from the C# basics page so one could assume that all the code snippets are in C#, therefore there's no need to indicate the language. In the singletons page we have snippets in GDScript too, so maybe the tab is useful as a language indicator idk.

Copy link
Contributor Author

@31 31 Jan 28, 2024

Choose a reason for hiding this comment

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

Yeah, language indicator is how I interpreted the single tab, and as long as we can't make whole sections swap, IMO it does its job fine. Interesting note about translation, didn't realize that but it makes sense.

@AThousandShips AThousandShips requested a review from a team January 28, 2024 00:14
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.

I think it's good to try and provide good examples of practical C# like this throughout the documentation. So I'm in favor of adding more C#-specific examples rather than 1-to-1 translations where we can (related: #6622).

tutorials/scripting/singletons_autoload.rst Outdated Show resolved Hide resolved
@raulsntos raulsntos added enhancement topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Jan 28, 2024
@31 31 force-pushed the dev/31/singleton-cs branch from c9b56bd to a4e958c Compare January 28, 2024 05:08
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.

Looks good to me, thanks!

@mhilbrunner mhilbrunner merged commit 62d5d6c into godotengine:master Feb 4, 2024
1 check passed
@mhilbrunner
Copy link
Member

Thank you!

@31 31 deleted the dev/31/singleton-cs branch February 5, 2024 03:44
@mhilbrunner
Copy link
Member

Cherry-picked to 4.2 in #9648.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation enhancement topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants