-
Notifications
You must be signed in to change notification settings - Fork 6.1k
F# Style Guide (new branch) #5313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
When you have static members, sometimes they mimic module functions. Having capitalization based on implementation details makes for naming inconsistencies when consuming API's in f#.
Also clarified the role of CompiledName attribute. You can see an examples of the attribute usage in https://github.com/fsharp/fsharp/blob/master/src/fsharp/FSharp.Core/option.fs#L52
Adding a section on static members
There was a problem hiding this 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, ![]()
| --- | ||
| title: F# component design guidelines | ||
| description: Learn the guidelines for writing F# components intended for consumption by other callers. | ||
| author: cartermp |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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...."
| ```fsharp | ||
| type Serializer = | ||
| type Serializer<'T> = | ||
| abstract Serialize<'T> : preserveRefEq: bool -> value: 'T -> string |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 threadsThread 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.
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