refresh LINQ docs to latest semantics #10113
Conversation
…tnet-api-docs into Linq-refresh-2024/7
dotnet-policy-service
bot
added
the
community-contribution
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
dotnet-issue-labeler
bot
added
the
needs-area-label
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
when I committed PR #10113 I got a "Run snippets 5000 for PR" warning due to file(s) without encompassing .csproj although the PR seemed to have gone through to review stage. Anyway this iteration is as fix [that step now passes]. Please accept & apply. Thanks. |
|
Learn Build status updates of commit ca1e55a: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
tarekgh
added
area-System.Linq
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-linq |
| @@ -0,0 +1,269 @@ | |||
| # Remove the line below if you want to inherit .editorconfig settings from higher directories | |||
There was a problem hiding this comment.
I defer to the docs owners for this, but if editorconfig settings are specified shouldn't they be set repo-wide?
There was a problem hiding this comment.
This file should be removed from the PR. The coding style should match the settings from the editorconfig at the root of the repo.
| { | ||
| OfTypeEx1(); | ||
| } | ||
| static void Main() => OfTypeEx1(); |
There was a problem hiding this comment.
If we're embracing modern C# shouldn't this be changed to use top-level statements instead?
There was a problem hiding this comment.
Yes. We'd prefer top-level statements.
We don't enforce the change on PRs, because we have many samples that pre-date top-level statements. However, if the purpose of this PR is to modernize the code, it should be top-level statements.
| @@ -4,19 +4,16 @@ | |||
|
|
|||
| namespace SequenceExamples | |||
There was a problem hiding this comment.
And possibly get rid of the namespace declarations where possible.
There was a problem hiding this comment.
Our style would be to use file scoped namespaces for larger samples. Using the global namespace is fine for smaller samples. (I'd call this a larger sample, personally).
| @@ -109,32 +106,29 @@ public static void AllEx() | |||
| static class All2 | |||
| { | |||
| // <Snippet129> | |||
| class Pet | |||
| class Pet(string name, int age) | |||
There was a problem hiding this comment.
Why specifically adopt a primary constructor in this example? The example could simply update the nullability annotation for Name or enforce immutability using init (and possibly required) instead of set if that's the end goal.
There was a problem hiding this comment.
I would prefer record or record struct types for this and other data types defined in these samples. The use (a data container) is the main scenario for record types. In the context of the sample, I don't have a strong preference for record class vs. record struct or enforcing immutability.
|
|
||
| // Determine which people have a non-empty Pet array. | ||
| IEnumerable<string> names = from person in people | ||
| where person.Pets.Any() | ||
| where person.Pets.Length != 0 |
There was a problem hiding this comment.
Given that these are snippets specifically intended to showcase use of Linq why not just keep Any?
There was a problem hiding this comment.
I'd prefer Any here as well.
| @@ -0,0 +1,61 @@ | |||
| | |||
There was a problem hiding this comment.
Seems to have been committed by mistake?
| @@ -1,8 +0,0 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
|
|
|||
| <PropertyGroup> | |||
There was a problem hiding this comment.
Curious why these project files are being deleted/renamed. If the projects are being restructured, what's the end goal here?
There was a problem hiding this comment.
the MS process mandates that there are .csproj files so that CI can validate. All good.
However it makes sense to knit *.csproj into a VS2022 .sln file so that all the analyzer improves can be applied together.
Having multiple x.csproj (same-name files e.g. program.csproj) is not acceptable to VS2022 so I renamed to unique.csproj [the *.csproj are invisible to the normal learn.microsoft.com/blah reader].
I agree with Eirik's other points of about top-level statements, NS (don't know how I missed those!)
But I lean towards analyzer recommendations [e.g. prefer .Count>0 or .Length>0 rather than Any() for perf]
having a splurge of Warnings/Messages is untidy and imho bad UX. Yes of course we are exhibiting LINQ so showing off lotsa members is good, but even good tools can be misused so "it depends" as ever.
I was not trying to sneak in contrary .editoconfig settings for long-term, but I despise "private" accessors as this is default and imho just contributes unnecessary noise to detriment of the code, i.e. I pick
dotnet_style_require_accessibility_modifiers = omit_if_default
but once I have cleaned-up such noise I am happy to revert to owner's version ["private" shouldn't re-appear]
I set the "let owner/admin modify" [sorry I can't remember exact phrase], so by all means give the changes another spin
I will watch reviewer comments with interest!
I appreciate the effort to update the doc snippets to portray latest C# semantics is a bore, but we all want such docs to give good guidance. Sometimes some ugly example throws me into digging up all the rabbit-holes [OCD!]
Anyway I am pleased the PR is now being looked at as otherwise my wasted effort crafting it!
There was a problem hiding this comment.
Hi @DickBaker
There are some great improvements here. But also a number of changes that should be addressed.
| @@ -0,0 +1,269 @@ | |||
| # Remove the line below if you want to inherit .editorconfig settings from higher directories | |||
There was a problem hiding this comment.
This file should be removed from the PR. The coding style should match the settings from the editorconfig at the root of the repo.
| @@ -4,19 +4,16 @@ | |||
|
|
|||
| namespace SequenceExamples | |||
There was a problem hiding this comment.
Our style would be to use file scoped namespaces for larger samples. Using the global namespace is fine for smaller samples. (I'd call this a larger sample, personally).
| { | ||
| OfTypeEx1(); | ||
| } | ||
| static void Main() => OfTypeEx1(); |
There was a problem hiding this comment.
Yes. We'd prefer top-level statements.
We don't enforce the change on PRs, because we have many samples that pre-date top-level statements. However, if the purpose of this PR is to modernize the code, it should be top-level statements.
|
|
||
| // Determine which people have a non-empty Pet array. | ||
| IEnumerable<string> names = from person in people | ||
| where person.Pets.Any() | ||
| where person.Pets.Length != 0 |
There was a problem hiding this comment.
I'd prefer Any here as well.
|
|
||
| double average = numbers.Average(num => long.Parse(num)); | ||
| double average = numbers.Average(long.Parse); |
There was a problem hiding this comment.
I'm ambivalent on this change. Less experienced programmers may be confused by using a method group instead of a lambda expression. If we keep this change, we should add a note in the corresponding text to explain a method group conversion.
|
|
||
| // Get the last item in the array, or else the default | ||
| // value for type string (null). | ||
| string last = fruits.AsQueryable().LastOrDefault(); | ||
| var last = fruits.LastOrDefault(); |
|
|
||
| Console.WriteLine( | ||
| String.IsNullOrEmpty(last) ? "[STRING IS NULL OR EMPTY]" : last); | ||
| last ?? "[STRING IS NULL OR EMPTY]"); |
There was a problem hiding this comment.
Same comment I had on the enumerable sample. This does change the semantics of the sample.
|
|
||
| Console.WriteLine("The last number that rounds to 50 is {0}.", last50); | ||
|
|
||
| // Get the last number in the array that rounds to 40.0, | ||
| // or else the default value for type double (0.0). | ||
| double last40 = | ||
| numbers.AsQueryable().LastOrDefault(n => Math.Round(n) == 40.0); | ||
| numbers | ||
| .LastOrDefault(n => Math.Round(n) == 40.0); |
There was a problem hiding this comment.
I'll stop commenting on it, but every sample in this file is to show a data source that uses IQueryable. The AsQueryable() calls should stay.
There was a problem hiding this comment.
not a fan as adds unnecessary overhead, and imho is bad guidance
but I will revert as-was to stay friends.
Yes I also prefer the lightweight record rather than class, so I will magic that in too.
|
|
||
| long count = fruits.AsQueryable().LongCount(); | ||
| long count = fruits.LongCount(); // [CA1829, RCS1077] better to use "fruits.Length;" |
There was a problem hiding this comment.
I'll stop commenting on this point, but all these should be removed when the purpose is to show the given extension method:
| long count = fruits.LongCount(); // [CA1829, RCS1077] better to use "fruits.Length;" | |
| long count = fruits.LongCount(); |
| @@ -109,32 +106,29 @@ public static void AllEx() | |||
| static class All2 | |||
| { | |||
| // <Snippet129> | |||
| class Pet | |||
| class Pet(string name, int age) | |||
There was a problem hiding this comment.
I would prefer record or record struct types for this and other data types defined in these samples. The use (a data container) is the main scenario for record types. In the context of the sample, I don't have a strong preference for record class vs. record struct or enforcing immutability.
dotnet-policy-service
bot
added
the
needs-author-action
current LINQ docs C# samples are vintage 2019 and need refresh (Issue #10110 relates)
obviously .NET and C# have evolved apace since, so imho opportune to modernise docs too for best guidance
This PR is intended to explain my cryptic notes in Issue #10110, and to propose a useful refresh of snippet code samples
Note that Allow edits and access to secrets by maintainers is checked so I will be happy for MS wonks to edit, e.g.
hopefully the time & effort I expended [as challenge to me!] will yield worthy update to the MS docs
good hunting !