Owin Extensions in F# - second review

[bennage]

Constants.fs

• Is there a reason to have RequestFormKey different from the others?
• I'm not sure about the visibility of these outside of the assembly

EnvironmentExtensions.fs

• The Helpers module is only there so that ParseForm can be visible for the unit test Otherwise, I would just make it a let inside the type.
• I don't think the type hosting the extension methods matter, so I named it x to keep it short since I have to reference it a lot internally
• I wanted to make these instance methods, as referenced in http://stackoverflow.com/questions/702256/f-extensions-in-c-sharp however it seems that I can't do that with constrained generic types such as IDictionary<'string,'Object>.

[bennage]

Yo @dahlbyk. You is mah ninja.

[half-ogre]

Is there a reason to have RequestFormKey different from the others?

The other keys are standard keys defined by the OWIN spec. The request form is our own key used to cache the parsed form. OWIN rules are that the key's prefix can't be owin. I used owinextensions because that's what I fancied naming these extensions if I was to ship them separately from QED.

I'm not sure about the visibility of these outside of the assembly

RequestFormKey should be visible to the outside world. The rest I don't care about.

---

One other thing: it feels like the ParseForm unit tests should be in F# and live in the F# assembly. Can you do xUnit.net tests in F#?

Other than those two things, I'd like to get a :thumbsup: from another pair of F# :eyes:, since I don't grok that code well enough yet. @dahlbyk, would you mind taking another last look at the code?

Thanks to you both!

[dahlbyk]

Also /cc @panesofglass given panesofglass/FSharp.Owin

[dahlbyk]

I'm not sure about the visibility of these outside of the assembly

Constants.fs compiles into roughly...

namespace Constants {
    public static class Owin {
        public static string CallCancelledKey { get { return "owin.CallCancelled"; } }
        ...
    }
}

The non-public members can be marked internal, but then the extension methods cannot be marked as inline since the constant references are not externally visible.

[dahlbyk]

There's a limit to how idiomatic one can be while targeting C# from F#, but this looks pretty good to my :eyes:

[panesofglass]

Also, anything you find to be missing in FSharp.Owin, I would happily add. That project is only partially started.

[forki]

One general comment. I'm really excited about this pull request and the QED project. :love_letter:

[half-ogre]

I'm really excited about this pull request and the QED project.

:heart_eyes:

(QED still has a really long way to go, but I'm pretty excited to.)

[panesofglass]

:+1: to QED!

Any chance you would like to merge some of these changes into FSharp.Owin?

[half-ogre]

@panesofglass I'll take a look at FSharp.Owin, but I'll be honest, it's going to be days or even weeks, as I'm super behind on everything right now. Getting QED solid comes first, but right after that for me is exploring writing OWIN apps and middlwares via F#, and I'll start by looking at what you've done.

[half-ogre]

@bennage Are VS 2013 F# projects not backwards compatibly to VS 2012? The project won't open for me.

[half-ogre]

[half-ogre]

Changing ToolsVersion to 4.0 seemed to work around it.

[forki]

They are backwards compatible, but one must do crazy changes sometimes. Is it fixed?

[half-ogre]

@forki Yeah, I fixed it.

@bennage Can you pull my changes to this branch and see if it still works for you in VS2013? Thanks!

[forki]

Bäm!

[half-ogre]

Thanks again for the contribution @bennage! I've made you a repo collaborator, so you can work directly in branches on this repo instead of needing to work in your own fork. Please read about how we work in this repository when you have the time. Thanks!