-
Notifications
You must be signed in to change notification settings - Fork 6k
Update information about C# 7 packages and ValueTuples #1842
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
Conversation
docs/csharp/whats-new/csharp-7.md
Outdated
> The new tuples features require the `System.ValueTuple` type. For Visual Studio 15 | ||
> Preview 5 and earlier preview releases, you must add the NuGet package "System.ValueTuple", | ||
> available in the pre-release stream. | ||
> The new tuples features require the `System.ValueTuple` type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk ... but this might render on two lines. Would it be safer to set it up as:
The new tuples features require the
System.ValueTuple
type. You must add the NuGet packageSystem.ValueTuple
in order to use it.
Also, how "new" is "new" when hardening the doc for the future? How do you feel about:
The tuples features require the
System.ValueTuple
type, and you must add the NuGet packageSystem.ValueTuple
in order to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole article is called "What's new in C# 7", so I think "new" will be appropriate here even in the future. Especially considering that there are "old" tuples (even though they don't actually have any language features supporting them).
docs/csharp/whats-new/csharp-7.md
Outdated
@@ -521,8 +520,8 @@ feature: | |||
[!code-csharp[UsingValueTask](../../../samples/snippets/csharp/new-in-7/AsyncWork.cs#30_UsingValueTask "Using ValueTask")] | |||
|
|||
> [!NOTE] | |||
> You need to add the pre-release NuGet package `System.Threading.Tasks.Extensions` | |||
> in order to use `ValueTask` in Visual Studio 15 Preview 5. | |||
> You need to add the NuGet package `System.Threading.Tasks.Extensions` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here ... I'd suggest just for safety against a crazy linefeed to put this on one line. It might not make a difference either way, but who knows. The 🙏 server gods 🙏 may smile on us if the note is on one line.
You need to add the NuGet package
System.Threading.Tasks.Extensions
in order to useValueTask
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point. This whole article uses newlines to keep the text lines at about the same length. And in Markdown, for a newline to matter, there either have to be two of them (in other words, an empty line) or the preceding line has to end with two spaces. Neither is the case here, so this newline shouldn't make a difference.
Also, this way, the diff is cleaner.
I'd like to get @richlander 's opinion on this set of changes. .NET Framework 4.7 just shipped, which (I think) has ValueTask and ValueTuple. I haven't verified yet, because my Win10 machine hasn't gotten the update yet. How would you want to phrase this now? |
I am looking at this topic for the first time, so will give feedback above and beyond the PR.
This feels like an apples to orangutan comparison. You can compare type to type and language support to no language support. ValueTuple and Tuple both have Item1 and Item2, so they don't feel different in that regard. If Tuple was a struct, I doubt the team would have created a new type. I think that the two changes are: language support is now available for tuples and is done so through a new type, created for performance reasons. It's possible that there are back-compat reasons, too, but I don't know that. Semantic names is a good description of the language support, however, I think of deconstruction as a separate additional feature.
Do we have a good sense of what the limitations where? It's not useful to make this claim unless they are given. The Item1, Item2 thing wasn't really a limitation as a type, for the reasons given above. Also, I'd call the old Tuple a "type" or "class". That's more descriptive, IMO.
I don't think this is a useful comparison, making specific reference to "Tuple syntax". You could say "which you can use in the same way as System.Tuple". There is no such thing, IMO, as "Tuple syntax", however.
I don't think of this as "modifying the assignment" but more about changing the C# syntax to take advantage of a language feature.
This shows up in a odd spot in the topic. It should show up as the first bit of content. To the question that @BillWagner raised, I'd suggest wording like the following:
I like the idea, which was suggested by @guardrex, to focus the text on the type. I think people will appreciate that
I get the use of "named" in "namedLetters". That said, some people might think this is guidance. We don't actually suggest that people start naming variables this way. I'd just keep with "letters".
This is very subtle. I think we need to explain that the field names are convenience names using some wording, as opposed to actual field names. I'd suggest "virtual name" but I think "virtual" already has a well established meaning. We might also need to explain that they are fields and not properties.
I think you can more simply say: "You can specify names ..." |
@BillWagner I have addressed most of the comments. @richlander Can you review if the changes are what you requested? Some comments follow.
How should that be reflected in the text? Especially considering that deconstruction is not really specific to
It's not just a single type, and I think "tuple types" would be confusing, so I left it as "tuples".
How would that work for .Net Core and .Net Standard, when they include Because of that, I made it vague:
I think it's useful to clearly differentiate
I think the main reason why people should know this is that the names are not really preserved at runtime, so that's how I explained it.
Why would this need a further explanation? It does say they are fields, and I don't think that's something that would be confusing. |
Wow. Thanks for all the work @svick |
Inspired by #1962, I have also added links to the packages on nuget.org to the notes. |
Rebased on top of current master due to conflict caused by #1962. |
is this ready to merge @guardrex and @richlander? |
Merging now. This is a major improvement, and already addresses some of the comments now in #1889. Once merged, we can examine those that still need work. |
I believe there are currently no released versions of .Net Framework/Core/Standard or VS which would include the packages by default, so I think it doesn't make sense to mention any versions in these notes.
cc: @BillWagner