Skip to content
This repository was archived by the owner on Nov 26, 2024. It is now read-only.

Conversation

@Indy9000
Copy link
Contributor

Simplified example

Simplified example
@dend
Copy link
Contributor

dend commented May 24, 2016

@KevinRansom @ReedCopsey can you take a look?

The sample is used in Arrays (F#) and Array.concat<'T> Function (F#). I believe tweaks to the sample will also require tweaks to docs to mention the output outside the code sample.

let multiplicationTable max = seq { for i in 1 .. max -> [| for j in 1 .. max -> (i, j, i*j) |] }
printfn "%A" (Array.concat (multiplicationTable 3)) No newline at end of file
Array.concat [ [|0..3|] ; [|4|] ]
//output [|0; 1; 2; 3; 4|]
Copy link
Contributor

Choose a reason for hiding this comment

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

Output should be placed in the doc instead of the code sample. I've included the links to the docs that reference this sample in the PR conversation thread.

@ReedCopsey
Copy link
Contributor

@dend The code is correct, but yes, the output should be included in the pages if you want to take this.

That being said, the previous code wasn't wrong in any sense - and in some ways, shows this off just as well as this simplified sample.

I do think that having this twice, in the same code block, would be incorrect. you'd need to include the printfn calls on both lines, which means the output would also be 2x as long.

@Indy9000 Perhaps this should be:

// Builds new array from list of arrays
printfn "%A" (Array.concat [ [|0..3|] ; [| 4 |] ])
// Builds new array from array of arrays
printfn "%A" (Array.concat [| [|0..3|] ; [| 4 |] |])

With the output block being:

[|0; 1; 2; 3; 4|]
[|0; 1; 2; 3; 4|]

@Indy9000
Copy link
Contributor Author

Indy9000 commented May 25, 2016

  1. I think the samples should minimally demonstrate the API behaviour. The original sample was correct but failed at this.

  2. If you have the result of the operation right next to it, it is unambiguous and convenient (for example when there are multiple lines of output, having to mentally parse 'oh the first three lines are from this printf and then ... ' ).

    2.a. It is easier for someone to update the docs with code samples.
    2.b. Printf is noise that the user has to filter out.

@dend
Copy link
Contributor

dend commented May 25, 2016

@Indy9000

  1. Totally agree with this. We have this problem with a lot of docs, so as long as the code shows the capabilities of the API in the simplest way possible, that is OK with me.
  2. Here I disagree. Arguably, it's easier to parse when I know for a fact where the output is presented, hence the Output section within the docs. Snippets should be code, and all additional information should live within the document. As long as the snippets are, like you said, simple and demonstrate the capabilities of the API, this should be OK.

@ReedCopsey
Copy link
Contributor

@Indy9000 I too agree with 1. - I was just pointing out that it wasn't "wrong" before as much as "more complex than necessary" - however, using a sequence does show that it works with anything, so in that one respect, it's nice.

WRT 2. - I'd actually agree, if the rest of the docs were that way, but I think it needs to fit the format of all of the other docs, which have the results in a separate block.

@dend
Copy link
Contributor

dend commented May 25, 2016

Before we start modifying all docs to a different convention, let's follow the existing structure regarding the output.

@dend
Copy link
Contributor

dend commented May 31, 2016

@Indy9000 could you please move the output in the doc itself to make sure that we follow the doc guidelines? That way I can merge this PR.

@dend
Copy link
Contributor

dend commented May 31, 2016

Got the #131 ready to match this. Thanks @Indy9000

@dend dend merged commit 2b0634d into MicrosoftDocs:live May 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants