Add user-facing docs for Variant #38712
Conversation
annelo-msft
requested review from
JoshLove-msft,
christothes and
KrzysztofCwalina
as code owners
annelo-msft
requested review from
jsquire,
heaths,
pallavit,
scottaddie,
tg-msft,
AlexanderSher and
m-nash
|
API change check API changes are not detected in this pull request. |
| } | ||
| ``` | ||
|
|
||
| If you do know the type the `Variant` is holding, you can use the `As<T>` method to get its value as that type. |
There was a problem hiding this comment.
As a potential caller, I'm not clear on whether the explicit cast operation, As<T>, or both will properly downcast or upcast e.g., Variant holds an int but would either approach upcast to a long? Or maybe it holds a long but small enough to fit into an int: will that be supported? What happens on overflow? I'd love to see these details explained, even if briefly, to know how to better use Variant.
There was a problem hiding this comment.
Oh. You do explain that As<T> doesn't cast just below. But, still, my question stands about the explicit cast operator. And should we support that if not supported already? I see a lot of advantage in some scenarios rather than a potentially long switch statement like you show above.
There was a problem hiding this comment.
Good questions - let me clarify this!
|
|
||
| `Variant` handles nullable primitives without boxing them for value types on the supported list above. It also supports holding a reference type with a `null` value. In either of these cases, the variant's `Type` property will return `null`. | ||
|
|
||
| Working with nulls with Variant can be a little tricky for the following reasons. First, since a Variant instance holding `null` has a value (i.e., it's a Variant holding a `null`), any comparison to `null` will return false. Second, assigning a `Variant` to an instance that holds `null` could require you to call the `Variant` constructor and cast the `null` some type to avoid ambiguous method resolution errors. Because of this, there are some APIs that make working with `null` and `Variant` a little easier. |
There was a problem hiding this comment.
Grammar hound time 😄 : should you put "(i.e., it's a Variant holding a null)" in parens if you already preface it with "i.e.,"? I would think it'd just be:
| Working with nulls with Variant can be a little tricky for the following reasons. First, since a Variant instance holding `null` has a value (i.e., it's a Variant holding a `null`), any comparison to `null` will return false. Second, assigning a `Variant` to an instance that holds `null` could require you to call the `Variant` constructor and cast the `null` some type to avoid ambiguous method resolution errors. Because of this, there are some APIs that make working with `null` and `Variant` a little easier. | |
| Working with nulls with Variant can be a little tricky for the following reasons. First, since a Variant instance holding `null` has a value i.e., it's a Variant holding a `null`, any comparison to `null` will return false. Second, assigning a `Variant` to an instance that holds `null` could require you to call the `Variant` constructor and cast the `null` some type to avoid ambiguous method resolution errors. Because of this, there are some APIs that make working with `null` and `Variant` a little easier. |
Alternatively, remove "i.e.," and just have it in parens. More of a question, though. I've never seen both used this way.
There was a problem hiding this comment.
Happy to fix - I always have to fix up my grammar over multiple drafts 🙂
| To assign the value a Variant holds to `null`, you can set it to `Variant.Null`: | ||
|
|
||
| ```csharp | ||
| Variant v = Variant.IsNull; |
There was a problem hiding this comment.
You document Variant.Null but show Variant.IsNull. Not sure which is right given I haven't looked at the code in a while.
There was a problem hiding this comment.
Good catch! Will fix - thanks!
|
|
||
| ## Handling nulls | ||
|
|
||
| `Variant` handles nullable primitives without boxing them for value types on the supported list above. It also supports holding a reference type with a `null` value. In either of these cases, the variant's `Type` property will return `null`. |
There was a problem hiding this comment.
The way I read this, any nullable value type will have a Type property value of null. Is that correct, or is it only when the value of the nullable value type is currently null?
There was a problem hiding this comment.
Ah, good. Let me clarify this. IIRC, the Type property is only null for a nullable value type when the value of the Nullable is null.
Before I checked, I had originally written that for value types we would return the actual type from the Type property (e.g. System.Nullable<System.Int32>), but for reference types we don't. I think this would be easy to implement but we don't do it today. We aren't able to store the type for reference types because it's either entirely unknown or would exceed our 128 bit storage limit.
What do you think? Would that be a better API experience?
There was a problem hiding this comment.
I do think it would be much better to return the actual nullable type, if possible. The alternatives both seem worse (e.g. Type's value can change depending on the value, or all nullable value types return null)
There was a problem hiding this comment.
Thinking to COM's VARIANT, though, this is analogous to VT_NULL. Sure, it's an int?, but because it's null does it even have a type? And what can any caller do with it besides treat it as null? Or do you feel there's sufficient coverage with the IsNull property?
There was a problem hiding this comment.
I do think it would be much better to return the actual nullable type, if possible.
I agree with this. Let me look at a) how hard it is to implement and b) what the implications are for type switching. I think in cases where we come in through a specific constructor (e.g. int? i = null; Variant v= i;), we can be clear about the type, and even though the value is null, if someone tried to do v = new MemoryStream() we could yell at them for type switching. It feels like there is a user intent here that we could support through the API. I'm not deep on VARIANT, but I know that people say it causes them nausea, so I don't think we have to preserve those rules here if we think there are better APIs to follow in a typesafe world.
I'm happy to look at examples where this gets interesting though, so we can think about whether a design choice like this might paint us into a corner. What are the pro/con cases here?
| @@ -0,0 +1,135 @@ | |||
| # Azure.Variant | |||
|
|
|||
| `Azure.Variant` is an implementation of a [tagged union](https://en.wikipedia.org/wiki/Tagged_union). It can hold both reference and value types and can be used to avoid boxing .NET primitives. The list of value types that Variant can hold without boxing is below. | |||
There was a problem hiding this comment.
Omit the locale from the URL. Also, avoid using directional terms like "below".
| `Azure.Variant` is an implementation of a [tagged union](https://en.wikipedia.org/wiki/Tagged_union). It can hold both reference and value types and can be used to avoid boxing .NET primitives. The list of value types that Variant can hold without boxing is below. | |
| `Azure.Variant` is an implementation of a [tagged union](https://wikipedia.org/wiki/Tagged_union). It can hold both reference and value types and can be used to avoid boxing .NET primitives. The following list contains value types that `Variant` can hold without boxing. |
| int i = (int)v; | ||
| ``` | ||
|
|
||
| If you don't know the type of the value that a `Variant` is holding, you can read that from the `Variant.Type` property. |
There was a problem hiding this comment.
| If you don't know the type of the value that a `Variant` is holding, you can read that from the `Variant.Type` property. | |
| If you don't know the type of the value that a `Variant` is holding, you can read that from the `Variant.Type` property: |
| MemoryStream streamValue = v.As<MemoryStream>(); | ||
| ``` | ||
|
|
||
| If you call `Variant.As<T>` and the `Variant` isn't holding the type that you ask for, it will throw an `InvalidCastException`. To try to retrieve the value without risking an exception being thrown, you can use the `TryGetValue` method instead. |
There was a problem hiding this comment.
| If you call `Variant.As<T>` and the `Variant` isn't holding the type that you ask for, it will throw an `InvalidCastException`. To try to retrieve the value without risking an exception being thrown, you can use the `TryGetValue` method instead. | |
| If you call `Variant.As<T>` and the `Variant` isn't holding the type that you ask for, it will throw an `InvalidCastException`. To try to retrieve the value without risking an exception being thrown, you can use the `TryGetValue` method instead: |
| } | ||
| ``` | ||
|
|
||
| If you do know the type the `Variant` is holding, you can use the `As<T>` method to get its value as that type. |
There was a problem hiding this comment.
| If you do know the type the `Variant` is holding, you can use the `As<T>` method to get its value as that type. | |
| If you do know the type the `Variant` is holding, you can use the `As<T>` method to get its value as that type: |
| If you do know the type the `Variant` is holding, you can use the `As<T>` method to get its value as that type. | ||
|
|
||
| ```csharp | ||
| MemoryStream s = new(); |
There was a problem hiding this comment.
Observation: Our team is very inconsistent with how we instantiate in our code samples. Here, the target-typed new expression is used. In other locations, this same line of code may have been written in one of the following forms:
MemoryStream c = new MemoryStream();var s = new MemoryStream();
This would be a good topic for the next .NET team sync. /cc: @jsquire
There was a problem hiding this comment.
Actually, it was already codified a long time ago in our root .editorconfig and is that var is only usable when the rvalue is obvious - most often instantiation.
Though, we never have discussed the new() pattern.
There was a problem hiding this comment.
My personal preference is for not using var because so often I don't know what type a method is returning and I can't mentally parse the code until I replace those, which I can't do on a GH page. I'm entirely agnostic on new() vs. new TypeName(), and happy to vote with the majority on this. In this example, I used new() just to reduce the width of the sample.
| Since you can't have an implicit cast operator from `object`, for reference types, you must create a new instance of `Variant` and pass the value to the `Variant` constructor. | ||
|
|
||
| ```csharp | ||
| MemoryStream s = new(); |
There was a problem hiding this comment.
Do we want to include a using statement in this sample?
| MemoryStream s = new(); | |
| using MemoryStream s = new(); |
There was a problem hiding this comment.
Good callout. I don't want to add complexity to the samples, so I'll pick a type for the examples that isn't disposable to sidestep the issue. Thanks, @scottaddie!
| } | ||
| ``` | ||
|
|
||
| ## Handling nulls |
There was a problem hiding this comment.
Avoid gerunds in headings.
| ## Handling nulls | |
| ## Handle nulls |
There was a problem hiding this comment.
What's the thinking behind this rule?
There was a problem hiding this comment.
I won't die on this hill, but there are 3 reasons:
- It's more difficult for non-English speakers to parse gerunds, since many of their languages don't have this word form. This factor matters if/when the content is localized. For example, if we decide to publish this content to learn.microsoft.com down the road.
- Gerunds are longer.
- The indicative verb form is the most direct (create a database vs. creating a database).
|
Hi @annelo-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
bot
added
the
no-recent-activity
|
Hi @annelo-msft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
|
/reopen |
github-actions
bot
removed
the
no-recent-activity
|
Hi @annelo-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
bot
added
the
no-recent-activity
|
/reopen |
annelo-msft
removed
the
no-recent-activity
|
Hi @annelo-msft. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
bot
added
the
no-recent-activity
|
Hi @annelo-msft. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
Addresses #32978
This PR adds samples for Variant in the Azure.Core.Experimental/samples directory, with the intention that these would move to Azure.Core/samples when Variant GA's in Azure.Core.
One of the purposes of this PR is to ask the questions: