fsprojects / FSharpx.Extras

Functional programming and other utilities from the original "fsharpx" project
https://fsprojects.github.io/FSharpx.Extras/
The Unlicense
683 stars 146 forks source link

Clean out toy implementations #195

Closed rojepp closed 10 years ago

rojepp commented 11 years ago

I know the title sounds abrasive, but I don't mean to be. :)

There are toy implementations in Fsharpx. By that, I mean it was fun to implement for someone, but it solves nothing, or worse, leaves the user in a worse state.

My prime example of this is the File system type provider. I'm sure it was a nice evening of hacking just for the hell of it, but it is a horrible idea to generate types from non-static folders. This leaves the user in a worse state, because it gives the illusion of type safety. It compiles on your machine, so you're safe in the knowledge that it will work, but it may fail at run-time because a certain folder doesn't exist. Things like this undermines the type system.

Are there valid uses for this TP? Are there other features that need cutting?

I like Fsharpx, but my concern is that others will see things like this and have their anti-oss concerns confirmed (OSS is a playground) and dismiss all of Fsharpx, or even OSS.

jackfoxy commented 11 years ago

@rojepp You haven't offended me. It may be moving slowly (we all have day jobs), but there is a major effort in the collections/datastructures area going on. https://github.com/fsharp/fsharpx/issues/169 I'm working on what I hope will be considered professional versions of 6 immutable collections (hopefully pushed soon, that's just the start). The "toy" implementations (I think of them as R&D implementations) will eventually get spun off to a new namespace/project/Nuget package, FSharpx.Collections.Sandbox. A big differentiator in quality is documentation. Everybody wants to hack and nobody wants to write good quality documentation. At a minimum to be considered professional every function, type, and value available to users should have XML docs. Taking time to implement FSharpx.Formatting literate programming would be even better.

forki commented 11 years ago

I want to help to remove/clean the toy implementations.

This leaves the user in a worse state, because it gives the illusion of type safety. It compiles on your machine, so you're safe in the knowledge that it will work, but it may fail at run-time because a certain folder doesn't exist.

That's a problem which basically exists with all type providers (and all dynamic solutions). You write something and the schema (FileSystem) changes. So what? A good type provider will give you ways to react to this (not the FS TP).

The idea of the FileSystem type provider is more to give you intellisense on the files in your project. You won't misspell any image filenames in your MVC app with this thing. I think it can be useful if you use it for something like that.

But nobody does this at the moment so I have no problem with removing it and putting it on a blog.

What do other think?

colinbull commented 11 years ago

I personally think of FSharpx.Core as extensions to the FSharp.Core and to this end DataStructures, computation expressions, Async extensions ect... should be part of this library, however as @jackfoxy states documentation is the key and should be a criteria for entry, but this can be time consuming to review, and I don't think we want to stunt the growth of the project.

I like the idea of a sand-boxing with a 'staging' branch, cherry picking the stuff that is of high enough quality to be included on the master.

As for the TypeProviders these are in a separate library anyway. But maybe they should be moved to a different project all together, as they aren't really extensions to the FSharp.Core. In my mind they set out to solve a particular problem that is not general, to this end they are more F# 'applied' and often useful in 'real world' situations. Taking the FileSytem provider; I can see lots of uses for this in enterprises where most of the data is transferred via files and file systems. In this case I want it to break if the folder structure is not what I expect as other legacy systems maybe affected.

rojepp commented 11 years ago

@forki That does sound like a valid, if small, use case.

That's a problem which basically exists with all type providers (and all dynamic solutions).

Agreed. Some more than others, though. With TP's, I think it's a good idea to be careful not to undermine the type system. What worries me is that this specific TP might encourage users to hard-code i.e. 'C:\Program files (x86)\blabla' in their programs, and being that it is strongly-typed, neglect to add proper error handling. :)

forki commented 11 years ago

@colinbull related: https://github.com/fsharp/fsharpx/issues/196

forki commented 11 years ago

@rojepp that's not the use case we had in mind. That use case might make the TP look bad. So I'd argue it's not the TP's fault.

andrew-cherry-15below commented 11 years ago

Having looked around the FSharpx.Core stuff a lot as I've been learning F# it seems like quite a lot of things have found a home there as "better than nowhere". Does FSharpx really need a (fairly naive) JSON parser? A STM implementation that I can't find any documentation on? (Dead links in source). Some of these things, especially the latter, seem like they'd justify their own library if they were well documented and supported. Could they find maintainers?

forki commented 11 years ago

Regarding the JSON parser: there is a new version of it which we took out of the Freebase provider. It much faster and already merged into FSharp.Data https://github.com/tpetricek/FSharp.Data/blob/master/src/Common/Json.fs

rojepp commented 11 years ago

Maybe everything that FSharp.Data does should be taken out of FSharpX so efforts can be focused in one place?

forki commented 11 years ago

@rojepp : discussion in https://github.com/fsharp/fsharpx/issues/196 - please comment there

ovatsus commented 11 years ago

I think the problem is really missing documentation. What is and what isn't toy is very subjective. If the filesystem type provider had a sample/docs suggesting the MVC use case that @forki pointed out, and warning people to not use it in a bad way, it would be fine. OTOH, the MiniCSV, as is suggested by its name, it's really too simple, and as there are now better CSV type providers, we should consider removing it. In any case, I really don't think in a comprehensive lib like FSharpx, having a couple of less quality things will make users think bad of the whole package. But we could try use the approach that @tpetricek used on FSharp.Data of having an experimental branch and .Experimental package

7sharp9 commented 11 years ago

Some of the stuff in fsharpx comes from finding 'cool stuff' from peoples blogs and using fsharpx as a means of preserving the information. The problem is if there is not valid use cases for the implementations or at least cursory documentation they will simple be left to rust in peace. Specifically I was talking about the STM implementation which came from an article on fpish.

7sharp9 commented 11 years ago

For me fsharpx is about extending the core libs with useful extensions and adding new functional pieces and wrappers. I feel like type providers are a separate entity. I don't think building them and publicly releasing them should be taken lightly, just as the MS guidelines suggest. Bullet proof features.

andrew-cherry-15below commented 11 years ago

That was my assumption and it seems like there should be a "blessed" place for some of that stuff to go to stop it being lost in the sands of time, but without it going in to a project which (I assume) is intended to come with an assurance of "probably good for production".

Things like STM could be very cool indeed if well documented with some good samples etc. and well maintained, but for now somewhere like an FSharpx.Experimental.* family seems like a good idea? So initially something like FSharpx.Experimental.STM (which could have a nuget package, etc.) but making it very clear that you're relying on this at your own risk. If at some point it becomes considered "good enough" then it can make a transition to FSharpx.STM.

(I have a personal aversion to big libraries with very broad functionality, but I'm trying not to let that sway my feelings :)

Maybe anything else which is basically a "this came from some random (but good) blog post, and should get a home" could follow the same path?

ghost commented 11 years ago

Re FileSystemProvider - @forki would that mean only allowing relative paths (relative to config.ResolutionFolder, or a static parameter which is itself relative)? For app development, that would seem more reasonable than absolute paths

For absolute paths, the question seems to be whether it is useful in data scripting scenarios. I could see that it might be, but the proof would really be to see whether data-scripters use it or not.

That reminds me, @ZachBray asked me about how we resolve relative file references at compile time and run time in type providers, for example for the ConfigFile static parameter to SqlEntityConnection type provider (http://technet.microsoft.com/en-us/subscriptions/hh362322.aspx) .

Here is what we do in FSharp.Data.TypeProviders:

  1. At design-time (i.e. statically), a relative reference is resolved relative to config.ResolutionFolder. However we also optionally allow a ResolutionFolder static parameter. If given, we use that at design-time to resolve relative references to local resources. (This is useful when partitioning into multiple projects and the files/resources live in a common place)
  2. At runtime a relative reference is resolved as follows:
    • When config.IsHostedExecution is true (i.e. in F# Interactive), use the ResolutionFolder static parameter if given, otherwise use config.ResolutionFolder
    • Otherwise, use CurrentDomain.BaseDirectory.TrimEnd('\', '/')

Some pseudo code for the type provider implementation that implements this is below

  1. the makeAbsolute function is a utility to make things absolute
  2. the getAbsoluteDesignTimeDirectory function computes the design-time directory as an absolute path (given the value for the ResolutionFolder static parameter, if any)
  3. the mkDefaultRuntimeResolutionFolderExpr function gives an expression to use at runtime for the default runtime resolution folder:
  4. the mkAbsoluteFileNameExpr gives an expression to use for the file name at runtime for a given static file reference

Putting it all together gives you something like this:

open System open System.IO open Microsoft.FSharp.Core.CompilerServices open Microsoft.FSharp.Quotations

[] type public MyProvider(config:TypeProviderConfig) =

let makeAbsolute (resolutionFolderParam, pathName) = 
    if String.IsNullOrWhiteSpace pathName then pathName 
    elif Path.IsPathRooted pathName then Path.GetFullPath pathName
    else Path.GetFullPath(Path.Combine(resolutionFolderParam, pathName))

let getAbsoluteDesignTimeDirectory resolutionFolderParam =  
    if String.IsNullOrWhiteSpace resolutionFolderParam then 
        config.ResolutionFolder
    else 
        makeAbsolute (config.ResolutionFolder, resolutionFolderParam)

let makeAbsoluteWithResolutionFolder (resolutionFolderParam, pathName) = 
    let absoluteDesignTimeDirectory = getAbsoluteDesignTimeDirectory resolutionFolderParam
    makeAbsolute(absoluteDesignTimeDirectory, pathName)

let mkDefaultRuntimeResolutionFolderExpr resolutionFolderParam =  
    if config.IsHostedExecution then 
        Expr.Value (getAbsoluteDesignTimeDirectory resolutionFolderParam)
    else
        // the base directory always ends with a "\", remove it
        <@@ System.AppDomain.CurrentDomain.BaseDirectory.TrimEnd('\\', '/') @@>

let mkAbsoluteFileNameExpr (resolutionFolderParam, staticFileName:string) =  
    if Path.IsPathRooted staticFileName then 
        Expr.Value staticFileName
    else
        let baseDirectoryExpr = mkDefaultRuntimeResolutionFolderExpr resolutionFolderParam
        Expr.Call(typeof<System.IO.Path>.GetMethod("Combine",[| typeof<string>; typeof<string> |]), [ baseDirectoryExpr; Expr.Value staticFileName ])

Don

nrolland commented 11 years ago

as @jackfoxy states documentation is the key and should be a criteria for entry, but this can be time consuming to review, and I don't think we want to stunt the growth of the project.

Thanks to Formatting, what is a chore for some can be a joy for others. Those data structures exists for a reason, and I am sure a few would like to investigate them and write illustrative case. Now this can be turned into very nice 'publishing material' for people with blogs (and boosting FSharp credentials to others along the way)

To do so in a useful way for FSharpx, we'd probably need some cohesion among the writers to follow some common pattern of documenting, so the original point is quite valid in that perspective. Guidance, like what Don did for path resolution is essential, so if anyone has a vision, please lay it out

As for now, my idea to make documentation less of a pain would be :

That does not solve the nice traditional exhaustive XML documentation. But with literate programming, don't we have some other ways to provide information ? Like finding exactly all the usage of a particular method across a set of open source librairies, including the documentation itself of course.

It looks feasible to me but I dont know how easy/hard it is given the available stuff.

jackfoxy commented 11 years ago

I haven't tested it, but I think traditional XML documentation and FSharp.Formatting will co-exist quite nicely.

@nrolland I'm a big fan of FSharp.Formatting and like your idea on how to integrate literate documentation into FSharpx, but that does not get documentation into the native VS help viewer. I think it can be done through SandCastle http://sandcastle.codeplex.com/ , but I have never investigated very far.Supporting two html doc formats is about impossible, so this is a dilemma.

But for now we should strongly encourage "exhaustive XML documentation" on all pull requests, and I call on those who have made past contributions to complete them with XML documentation. There is a lot of really good stuff in FSharpx that suffers from lack of XML/intellisense documentation.

7sharp9 commented 11 years ago

In the fsi file or main file? I like it in the interface files so it doesn't clog up the main code.

jackfoxy commented 11 years ago

@7sharp9 in the fsi (take a look at the recent additions to core/collections), but there's a bunch of stuff in core, and even in collections, that has no XML doc whatsoever. Even if its use is obvious, intellisense provides a level of comfort. If there is fsi I put only "normal" comments in the fs that might be of interest to someone looking at the code.

nrolland commented 11 years ago

@jackfoxy interesting.. and also puzzling why they abandoned that ! That's what happens when you don't have enough F#ers.

As we discuss about extending documentation, we'll need tooling, fake support down the road. Could you guys look at the following issue about extending Fake and tell me what do you think ?

https://github.com/fsharp/FAKE/issues/89

tpetricek commented 11 years ago

F# Formatting is designed for writing tutorials, while traditional XML documents are much better for building API references. I think tutorials are important to get people started, but having a proper API reference is important too, especially for larger libraries.

I think it would be nice if the generation of API referneces (from XML files) was integrated into F# formatting too - so if anybody has ideas in that direction and time to implemented, let me know! (To be a bit radical, I think replacing the XML with simple Markdown-based format would be good).

7sharp9 commented 11 years ago

@tpetricek You hit the nail on the head I was thinking about inline markdown the other day. Make it so!

Full xmldoc comments are awful, so cluttered and ugly.

jackfoxy commented 11 years ago

@tpetricek The suite of tools in FSharp.Formatting is so good (and so well documented), people are naturally going to stretch beyond your original intent. Right now (as I interrupt my work to keep up on this thread) I'm using the literate tools to create the ReadMe of a prototype system for a client.

forki commented 11 years ago

Hi,

To be a bit radical, I think replacing the XML with simple Markdown-based format would be good)

I proposed this http://tom.preston-werner.com/2010/05/11/tomdoc-reasonable-ruby-documentation.html a while ago. I think it should be part of the F# Compiler.

Cheers, Steffen

jackfoxy commented 11 years ago

I may be abusing the term "intellisense", but the first order of documentation IMHO is the pop-up on mouseover and upon entering "dot" in Visual Studio (and any other IDE that can do this). Whether this is done through XML, Markdown, of something else is less important.

In fact I vote +1 complete intellisense should be a requirement for pull requests to Core (but not a requirement to Experimental).

Markdown and/or Literate doc of any format would be nice to have, but if contributors don't even write enough documentation to feed intellisense, how can we expect them to write entire documents?

panesofglass commented 11 years ago

I think it may be time for a new major version rev with breaking changes. We should move stable, core stuff into Core, and pull out experimental or otherwise questionable stuff into an Experimental project, similar to the way Rx broke up their early work. Type Providers are already split out. Most of the Data Structures are probably Core. I think we may have a few duplicates. One item in particular I'd like to remove is iteratees. I have no idea if anyone is using that, but a major version revision gives us the opportunity to tear some of that stuff out.

(And yes, I'm currently using iteratees, but I would like to break into something new. See #209.)

jackfoxy commented 11 years ago

Agreed, time for major rev. DataStructures is already marked obsolete, superseded by Collections.Experimental. About a half dozen structures in Experimental are good candidates for Core.Collections after some polishing including XML doc. I personally prefer an organization with Experimental projects, rather than an Experimental branch.

fsgit commented 10 years ago

Closing this as it has generally been resolved by splitting out FSharpx.Collections and other items