Skip to content

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

Merged
merged 3 commits into from
May 22, 2017

Conversation

svick
Copy link
Contributor

@svick svick commented Apr 5, 2017

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

> 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.
Copy link
Contributor

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 package System.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 package System.ValueTuple in order to use it.

Copy link
Contributor Author

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).

@@ -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`
Copy link
Contributor

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 use ValueTask.

Copy link
Contributor Author

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.

@BillWagner
Copy link
Member

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?

@richlander
Copy link
Member

I am looking at this topic for the first time, so will give feedback above and beyond the PR.

[!NOTE] Tuples were available before C# 7 as an API, but had many limitations. Most importantly, the members of these tuples were named Item1, Item2 and so on. The language support enables semantic names for the fields of a Tuple.

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.

Tuples were available before C# 7 as an API, but had many limitations.

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.

That assignment creates a tuple whose members are Item1 and Item2, following the existing @System.Tuple syntax.

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.

You can modify that assignment to create a tuple that provides semantic names to each of the members of the tuple:

I don't think of this as "modifying the assignment" but more about changing the C# syntax to take advantage of a language feature.

The new tuples features require the System.ValueTuple type.

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:

You need the TypeA type in order to use Language Feature B. It is included in .NET Framework x.y. When targeting earlier .NET Framework versions, you need to install NuGet Package C.

I like the idea, which was suggested by @guardrex, to focus the text on the type. I think people will appreciate that

(string Alpha, string Beta) namedLetters = ("a", "b");

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".

The namedLetters tuple contains fields referred to as Alpha and Beta.

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.

The language allows you to specify names for the fields on both the left and right-hand side of the assignment:

I think you can more simply say: "You can specify names ..."

@BillWagner
Copy link
Member

BillWagner commented Apr 12, 2017

@svick Rich's comments go beyond what you've submitted here. I've created issue #1889 to capture those comments. Let us know if you want to expand the scope of what you're doing here. If not, we'll take this PR as is and leave the other issue open.

@svick svick changed the title Update information about C# 7 packages Update information about C# 7 packages and ValueTuples Apr 13, 2017
@svick
Copy link
Contributor Author

svick commented Apr 13, 2017

@BillWagner I have addressed most of the comments.

@richlander Can you review if the changes are what you requested? Some comments follow.

I think of deconstruction as a separate additional feature

How should that be reflected in the text? Especially considering that deconstruction is not really specific to ValueTuple.

Also, I'd call the old Tuple a "type" or "class".

It's not just a single type, and I think "tuple types" would be confusing, so I left it as "tuples".

I'd suggest wording like the following:

You need the TypeA type in order to use Language Feature B. It is included in .NET Framework x.y. When targeting earlier .NET Framework versions, you need to install NuGet Package C.

How would that work for .Net Core and .Net Standard, when they include ValueTuple in the future? Should it say something like "It is included in .NET Framework 4.7, .NET Core 2.0 and .NET Standard 2.0"? And even now, are people going to understand that .Net Core 1.x and .Net Standard 1.x are "earlier .NET Framework versions"?

Because of that, I made it vague:

You must add the NuGet package System.ValueTuple in order to use it on platforms that do not include the types.

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".

I think it's useful to clearly differentiate letters from the first snippet with namedLetters from the second snippet, so I think the names should stay different.

I think we need to explain that the field names are convenience names using some wording, as opposed to actual field names.

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.

We might also need to explain that they are fields and not properties.

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.

@BillWagner
Copy link
Member

Wow. Thanks for all the work @svick
I reviewed the changes and I'd be happy to merge. @richlander What's your opinion?

@svick
Copy link
Contributor Author

svick commented Apr 19, 2017

Inspired by #1962, I have also added links to the packages on nuget.org to the notes.

@svick
Copy link
Contributor Author

svick commented Apr 20, 2017

Rebased on top of current master due to conflict caused by #1962.

@mairaw
Copy link
Contributor

mairaw commented May 1, 2017

is this ready to merge @guardrex and @richlander?

@BillWagner
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants