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

Docs: Note that C# does not have "copy constructors" for variant types #98570

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tetrapod00
Copy link
Contributor

Closes godotengine/godot-docs#10148.

This PR notes that constructors of the form public Basis(Basis from) are not available in C#.

To create a copy of a struct variant type in C#, the following syntax with a "copy constructor" is not available:

Basis original = Basis.Identity;
Basis copy = new Basis(original);

Instead, the following syntax can be used:

Basis original = Basis.Identity;
Basis copy = original;

@tetrapod00 tetrapod00 requested a review from a team as a code owner October 27, 2024 00:39
@AThousandShips AThousandShips added enhancement documentation discussion cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Oct 27, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Oct 27, 2024
@Mickeon Mickeon requested a review from a team October 27, 2024 17:12
Copy link
Member

@paulloz paulloz left a comment

Choose a reason for hiding this comment

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

This is fine from the C# side. Although, I'm personally not convinced how useful it is, given this is very standard struct behaviour.

@AThousandShips
Copy link
Member

I think this might not be the appropriate place to put this, while we do have suggested changes like #98499 this is very much a part of the general differences with C# and I think belongs in the dedicated C# differences page

@tetrapod00
Copy link
Contributor Author

Yeah, I do think this change is pretty borderline. If someone didn't go to the effort of making a docs issue, I wouldn't have made this PR. But clearly existing users do use the online class ref for C#, and they are confused by the differences in API when they exist and are not documented.

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.

I'm skeptical on adding this, as all of these repetitive notes are a burden to maintain and localize. Although there are precedents to this (which I also don't like, personally).

Obviously I do think it has a place in the class reference, but perhaps it should be an automatically generated note, instead. That would make it similar to #85207 , which also originally started as a spam of notes.

doc/classes/AABB.xml Outdated Show resolved Hide resolved
@tetrapod00 tetrapod00 force-pushed the csharp-missing-constructor branch from d404583 to ccb51a3 Compare October 28, 2024 18:16
@tetrapod00
Copy link
Contributor Author

tetrapod00 commented Oct 29, 2024

I made the changes, so if this should be merged it can be. But I think we're deferring this until it's implemented as an automatically generated note, to reduce future maintenance?

In the meantime I'll look into a smaller docs change on the C# API differences manual page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release discussion documentation enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No constructor for basis as a copy of another basis
4 participants