Skip to content

Conversation

@31
Copy link
Contributor

@31 31 commented Jan 28, 2024

I took a peek at this doc for reference for #8815 (comment) and saw that private Foo _foo style wasn't being used consistently here. Also a couple minor grammar changes.

@skyace65 skyace65 added bug topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.2 labels Jan 28, 2024
@AThousandShips
Copy link
Member

I think these should be public instead as they're exported

CC @raulsntos

@AThousandShips AThousandShips requested a review from a team January 28, 2024 20:06
@paulloz
Copy link
Member

paulloz commented Jan 28, 2024

We use private exported fields in a lot of places, and we explicitly discourage using public fields in the style guide.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 28, 2024

Where? The styleguide makes no mention of exports, and the exports page uses public almost all the time

The official demo projects also do not use private fields exclusively

A search of the documentation itself shows:

  • 13 cases of private
  • 92 cases of public

A search of the demo projects shows:

  • 2 cases of private
  • 6 cases of public

@paulloz
Copy link
Member

paulloz commented Jan 28, 2024

The styleguide makes no mention of exports

Didn’t say the styleguide mentioned exports, it mentions preferring properties to non public fields. And yes, the docs also show a lot of exported public properties. Which is why I’m saying if this is intended to be shown as public, it should be a property, else it should stay private. And if it is shown as a property, we essentially have a page about exports that lacks any example using fields which is… misleading IMHO.

Then again, only trying to help.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 29, 2024

Gotcha, did mean public and fields, not necessarily public plain variables 🙂 sorry for being unclear

@31
Copy link
Contributor Author

31 commented Jan 29, 2024

I think these should be public instead as they're exported

Personally, I agree. [Export] to me is simply annotating a piece of my public API saying that Godot should show it in the inspector.

However, many people I've talked with (e.g. on the C# help discord channel) are accustomed to C# serialization libraries that readily get and set private state. From this perspective, [Export] private int _x; is a form of encapsulation: it allows Godot to change these members while protecting them from inadvertent changes by other C# scripts that interact with them.

I think it's important for this doc to point out that fields and properties, public and private, all work, so nobody thinks Godot is "forcing" them to do something they feel is unnatural--regardless of Godot or broader industry style.

That said, making all but the first few examples default to public Foo Foo { get; set; } doesn't seem bad to me as long as it's still obvious that private fields/properties work.

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.

Making them public makes sense to me. As long as, like you said, it's clear that it's also possible to export fields, and that the access modifier doesn't matter.

I think, with the current changes, it's clear enough.

@31 31 force-pushed the dev/31/clean-export-cs branch from 0a85ca4 to b3b15a6 Compare January 30, 2024 07:38
@mhilbrunner mhilbrunner merged commit 07cb95a into godotengine:master Jan 30, 2024
@mhilbrunner
Copy link
Member

Thank you! Merged. 🎉

@31 31 deleted the dev/31/clean-export-cs branch January 31, 2024 00:15
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 bug cherrypick:4.2 topic:dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants