Skip to content

Conversation

@MecuStefan
Copy link
Contributor

No description provided.

@dnfadmin
Copy link

dnfadmin commented Sep 8, 2021

CLA assistant check
All CLA requirements met.

@baronfel
Copy link
Member

baronfel commented Sep 8, 2021

Adding a link to the meta-issue for tracking. Thanks for submitting these!

Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

This is a fantastic start - the examples are really well chosen.

Have left some comments - could you aplpy them systematically and I'll review again ?

///
/// <returns>The concatenated string.</returns>
///
/// <example> The following samples shows how to interspace spaces in a text
Copy link
Member

@baronfel baronfel Sep 8, 2021

Choose a reason for hiding this comment

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

This is actually the first time I've seen multiple examples in the wild, so congratulations on surprising me :)

I did check, and FSharp.Formatting and FsAutoComplete do handle multiple examples just fine, so this shouldn't be a problem from the perspective of tooling consuming these docs.

@dsyme it does make me want some kind of 'example identifier' for use in generation, for example generating a link of FSharp.Core.StringModule#collect-example-collect-space in order to jump to the specific example, but that's just some off-the-cuff musing. (FSharp.Formatting already generates a numeric identifier for this case, but something more stable might be useful?)

Copy link
Contributor

Choose a reason for hiding this comment

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

@dsyme it does make me want some kind of 'example identifier' for use in generation, for example generating a link of FSharp.Core.StringModule#collect-example-collect-space in order to jump to the specific example, but that's just some off-the-cuff musing. (FSharp.Formatting already generates a numeric identifier for this case, but something more stable might be useful?)

Makes sense, yes, even just collect-example-1

Copy link
Contributor

@dsyme dsyme Sep 8, 2021

Choose a reason for hiding this comment

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

just use <example id="collect-whatever"> I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I did check, and FSharp.Formatting and FsAutoComplete do handle multiple examples just fine, so this shouldn't be a problem from the perspective of tooling consuming these docs.

Thank you for checking this!!!!

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that's what I was getting at, I just wonder if it's worth codifying that in the tooling RFC and adding support to FSharp.Formatting then.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, that's what I was getting at, I just wonder if it's worth codifying that in the tooling RFC and adding support to FSharp.Formatting then.

Absolutely

Copy link
Member

Choose a reason for hiding this comment

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

tracking this on the RFC and in FSharp.Formatting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ids to every example tag

Copy link
Contributor Author

@MecuStefan MecuStefan Sep 8, 2021

Choose a reason for hiding this comment

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

Facepalm, I get it now: you want to generate the ids ... . I should remove them then?

Copy link
Member

Choose a reason for hiding this comment

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

no, I'd keep them. My intent was for the author to give them meaningful ids. There is some last-chance id generation happening in FSharp.Formatting, but the MR I have up now always takes the id that is present if there is one. So, all in all, I think what you have now is great 👍

@MecuStefan MecuStefan requested a review from dsyme September 8, 2021 20:58
Copy link
Contributor

@dsyme dsyme left a comment

Choose a reason for hiding this comment

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

I've left some comments/improvements - I'll merge and could you address them on next PR? Thanks

/// <code lang="fsharp">
/// let input = "Hello"
/// input |> String.iteri (fun i c -> printfn "%d. %c %d" (i + 1) c (int c))
/// // evaluates unit
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these english language parts should not be in <code>

///
/// <example id="init-example-1"> Enumerate digits ASCII codes
/// <code lang="fsharp">
/// String.init 10 (fun i -> int '0' + i |> sprintf "%d ") // evaluates "48 49 50 51 52 53 54 55 56 57 "
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are getting too long I think (102 characters), suggest a maximum of 80 with a preference for 60

        /// <code lang="fsharp">
        /// String.init 10 (fun i -> int '0' + i |> sprintf "%d ")
        /// </code>
        /// evaluates to
        /// <code>
        /// "48 49 50 51 52 53 54 55 56 57 "
        /// </code>

across all the samples.

/// <code lang="fsharp">
/// let input = "Hello"
/// input |> String.iter (fun c -> printfn "%c %d" c (int c))
/// // evaluates unit
Copy link
Contributor

Choose a reason for hiding this comment

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

English language parts outside of <code>

@dsyme dsyme merged commit 2cd818a into dotnet:main Sep 9, 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.

4 participants