Skip to content

Some examples for the List module. Small refactor of the String examples as requested by reviewer.#12134

Merged
dsyme merged 18 commits into
dotnet:mainfrom
MecuStefan:main
Sep 14, 2021
Merged

Some examples for the List module. Small refactor of the String examples as requested by reviewer.#12134
dsyme merged 18 commits into
dotnet:mainfrom
MecuStefan:main

Conversation

@MecuStefan

@MecuStefan MecuStefan commented Sep 9, 2021

Copy link
Copy Markdown
Contributor

Some examples for the List module. Small refactor of the String examples as requested by reviewer.

Tracking issue: #12124

@dsyme dsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a bunch of comments :)))

Nearly all the examples are really great. I think needs some additional work for fold/foldBack

Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
///
/// <example id="countBy-example-1">
/// <code lang="fsharp">
/// let isEven x = 0 = x % 2

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This example feels a bit too mathematical (modulus) - better to be getting a statistic of some kind, e.g. count a list of lists by length, or strings by number of occurrences of characters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Counted the letters in the ['H'; 'a'; 'p'; 'p'; 'i'; 'n'; 'e'; 's'; 's'] . Do you think that will be better to use "Happiness".ToCharArray() |> Array.toList ?

Comment thread src/fsharp/FSharp.Core/list.fsi
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
@dsyme dsyme mentioned this pull request Sep 14, 2021
@MecuStefan

Copy link
Copy Markdown
Contributor Author

Thank you Don for review!

Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
Comment thread src/fsharp/FSharp.Core/list.fsi Outdated
@dsyme

dsyme commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

Just a few more comments

@dsyme

dsyme commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

Thanks so much. Pushed some cosmetic changes, when it's green it'll merge

@dsyme dsyme enabled auto-merge (squash) September 14, 2021 22:16
@dsyme dsyme merged commit 108da53 into dotnet:main Sep 14, 2021
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.

2 participants