Skip to content

Conversation

@31
Copy link
Contributor

@31 31 commented Feb 2, 2024

This page deserves examples and a little correction about the implicit vs. explicit conversions.

The example about comparing two Variants using .Obj and == might seem oddly specific, but it's come up at least twice now in the C# discord help channel.

I also assumed a while back that .As<T> would behave more like a a as B, and the explicit conversion (B)a would throw an exception, but it's not the case (and the Packed{Type}Array empty array case is interesting), so worth mentioning IMO, but not sure in how much detail.

@AThousandShips AThousandShips requested a review from a team February 2, 2024 09:44
@AThousandShips AThousandShips added topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Feb 2, 2024
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 appreciate the effort to improve the Variant documentation. I want to be careful about how we explain the type to avoid encouraging its usage more than necessary, I hope I wasn't too nit-picky.

a ``Godot.Variant`` to a C# type. Since the ``Godot.Variant`` type contains implicit conversions
defined for all the supported types, calling these methods directly is usually not necessary.
Converting from a Variant-compatible C# type to ``Godot.Variant`` can be done using implicit
conversions. Also available are ``CreateFrom`` method overloads and the generic ``Variant.From<T>``
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
conversions. Also available are ``CreateFrom`` method overloads and the generic ``Variant.From<T>``
conversions. You also have ``CreateFrom`` method overloads and the generic ``Variant.From<T>``

Or

Suggested change
conversions. Also available are ``CreateFrom`` method overloads and the generic ``Variant.From<T>``
conversions. There are also ``CreateFrom`` method overloads and the generic ``Variant.From<T>``

"Also available are" sounds clunky in English IMO

Copy link
Contributor Author

@31 31 Feb 4, 2024

Choose a reason for hiding this comment

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

FWIW to me (en-us) it's a common phrase that implies the first (previous) option is the default. also available shows up elsewhere in the docs too, just not followed with are in particular. I do think There are also works fine though, happy to change it. (Which is now pushed.)

Co-authored-by: A Thousand Ships <96648715+AThousandShips@users.noreply.github.com>
@31 31 force-pushed the dev/31/variant-obj branch from 6c7d503 to c2e68a8 Compare February 4, 2024 01:17
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 looks great already, thank you for improving the C# Variant documentation. I only left a minor nit-pick.

@mhilbrunner mhilbrunner merged commit 664b739 into godotengine:master Feb 4, 2024
@mhilbrunner
Copy link
Member

Whoops, should have squashed. Anyway, merged! Thanks, and thanks for the reviews everyone.

@31 31 deleted the dev/31/variant-obj branch February 5, 2024 03:44
mhilbrunner added a commit that referenced this pull request Jul 24, 2024
c_sharp_variant.rst: add more examples, fix explicit vs. implicit

(cherry picked from commit 664b739)
@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 topic:dotnet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants