Skip to content

Conversation

@cartermp
Copy link
Contributor

The current PR (#4916) gives me a GitHub unicorn every time I click, to the point where I cannot address feedback with the github UI.

cc @dsyme @eiriktsarpalis @bartelink @selketjah @AnthonyLloyd

@cartermp cartermp requested review from BillWagner and mairaw May 11, 2018 17:41
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

This version looks great. I had a couple comments.

Once you look at those, :shipit:

---
title: F# component design guidelines
description: Learn the guidelines for writing F# components intended for consumption by other callers.
author: cartermp
Copy link
Member

Choose a reason for hiding this comment

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

general comment:

@mairaw has been updating our global and folder metadata. The only tags you need are "title", "description", and "ms.date". (You are the default author for the F# guide.

You can simplify all these files.


#### Use interfaces to group related operations

We recommend you use interface types to represent a set of operations. This is preferred to other options, such as tuples of functions or records of functions.
Copy link
Member

Choose a reason for hiding this comment

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

Avoid "we", unless this is official Microsoft guidance. Consider just "Use interface types...."

@cartermp cartermp merged commit 5b3d968 into dotnet:master May 14, 2018
```fsharp
type Serializer =
type Serializer<'T> =
abstract Serialize<'T> : preserveRefEq: bool -> value: 'T -> string
Copy link
Contributor

@bartelink bartelink May 14, 2018

Choose a reason for hiding this comment

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

The 'Ts need to go off the methods if they are to be at the type level.

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'll get a quick fix of that in there

|> Array.concat
// OK, but prefer previous
let methods2 =
Copy link
Contributor

Choose a reason for hiding this comment

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

re this:

// OK
let methods2 = System.AppDomain.CurrentDomain.GetAssemblies()
               |> List.ofArray
               |> List.map (fun assm -> assm.GetTypes())
               |> Array.concat
               |> List.ofArray
               |> List.map (fun t -> t.GetMethods())
               |> Array.concat

This first version is not a good convention in practice and I'd always remove it from the compiler code base. In particular code breaks under rename-refactor. (I think all the code formatting guidelines should be measured with this in mind).

I haven't read the history of the discussion, so I don't know we're you're at on this. But I'd recommend putting the second version first, and switching the " OK, but prefer previous"

Copy link
Contributor

@bartelink bartelink May 15, 2018

Choose a reason for hiding this comment

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

+alot I believe I commented to that effect at one point

My grounds for preferring indenting by one unit on next line are that unindenting when the hard-won indentation is not a multiple of your preferred indentation level often breaks compilation / varies the meaning in various contexts
(a not unimportant secondary justification is that the further code is indented, the harder it is to review in various contexts.)

Copy link
Contributor

@dsyme dsyme May 15, 2018

Choose a reason for hiding this comment

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

Similar is this kind of code:

     match x with 
     | Pat1,Pat2 -> { aaaaaa = 1
                      bbbbbb = 2 }
     | Pat1,Pat3 -> { aaaaaa = 1
                      bbbbbb = 2 }

Renaming Pat2 causes a build break. Again I would always reformat to

     match x with 
     | Pat1,Pat2 -> 
         { aaaaaa = 1
           bbbbbb = 2}
     | Pat1,Pat3 -> 
         { aaaaaa = 1
           bbbbbb = 2}

I just did a rename-refactor today (actually it was a whitespace replacement, replacing "," with ",<space>") that broke code like this.

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'll get a PR rolling

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'm going to just remove that first example. I'll want to do a deeper pass in this article as it's mostly just a transcription of the Fantomas doc.

@cartermp cartermp deleted the style2 branch May 15, 2018 16:31
module MyApi =
let dep1 = File.ReadAllText "/Users/{your name}/connectionstring.txt"
let dep2 = Environment.GetEnvironmentVariable "DEP_2"
let dep3 = Random().Next() // Random is not thread-safe
Copy link
Member

Choose a reason for hiding this comment

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

Would probably remove the reference to thread safety here. In the particular case this random value initialization will be inlined in a static constructor which is guaranteed to run precisely once by the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there's nothing intrinsically thread-unsafe about the code shown, though simple variations on it are not safe, e.g.

module MyApi =
    let dep3 = Random()
    let getRandom() = dep3.Next() // problems if getRandom is called from multiple threads

Thread safety and static data is a tricky topic: many, many F# programs are written single-threaded or use multi-threading in constrained ways (Array.Parallel). It's also possible to write thread-safe global services (lock or use concurrency-safe data structures and so on). So these things depend a lot on the overall programming context, the ability of the programmer etc.

Perhaps simpler guidance would be statically initialized data should not include values that are not thread safe if your component will itself use multiple threads.

In any case, it's not the presence of side effects in the static initialization code that's the real risk - it's the presence of non-thread-safe objects in the results.

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