Skip to content
This repository has been archived by the owner. It is now read-only.

Move NancyOwinHost to core. #1544

Merged
merged 5 commits into from Jun 11, 2014
Merged

Move NancyOwinHost to core. #1544

merged 5 commits into from Jun 11, 2014

Conversation

khellang
Copy link
Member

@khellang khellang commented May 25, 2014

I think it's time we address this and start a discussion on how to structure/package things moving forward, and what better way to do that than with a pull request? 馃槈

This PR renamesNancyOwinHost to NancyMiddleware and moves it to the core project, (still) under the Nancy.Owin namespace.
The AppBuilderExtensions are left intact in the Nancy.Owin project. API-wise, this is not even a breaking change 馃憤

I also added extensions for the Action<Func<AppFunc, AppFunc>> approach for adding middleware, used in the new IBuilder.UseOwin extensions. This means that you can now do

public void Configure(IBuilder app)
{
    app.UseOwin(owin => owin.UseNancy());
}

in ASP.NET vNext, without other dependencies than core Nancy 馃懐

Fixes #1387

@@ -0,0 +1,48 @@
锘縰sing System;
Copy link
Member

@jchannon jchannon May 25, 2014

Choose a reason for hiding this comment

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

Put usings under the namspaces declaration human stylecop strike

Copy link
Member

@grumpydev grumpydev May 25, 2014

Choose a reason for hiding this comment

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

zOMG! Good job @thecodejunkie isn't watching!

image

Copy link
Member Author

@khellang khellang May 25, 2014

Choose a reason for hiding this comment

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

Problem is then you have to declare the Funcs with full names, e.g.

using AppFunc = System.Func<System.Collections.Generic.IDictionary<string, object>, System.Threading.Tasks.Task>

So I think it's worth the tradeoff (what is the tradeoff, exactly? 馃槤)...

Copy link
Member

@grumpydev grumpydev May 25, 2014

Choose a reason for hiding this comment

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

Tbh, the funcs in owin are so much smaller these days I'm not sure I see the point in aliasing them.

Copy link
Member

@grumpydev grumpydev May 25, 2014

Choose a reason for hiding this comment

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

And the tradeoff is that putting them inside the namespace scopes them correctly, which can avoid issues with namespace collisions.. on the flip side, putting them outside of the namespace gets you stabbed in the belly by @thecodejunkie, who will grow a Sparta-eque beard just for the occasion.

Swings and roundabouts really.

Copy link
Member

@damianh damianh May 28, 2014

Choose a reason for hiding this comment

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

The latter is what turns up in intellisense though.
On 28 May 2014 09:26, "Kristian Hellang" notifications@github.com wrote:

In src/Nancy/Owin/DelegateExtensions.cs:

@@ -0,0 +1,48 @@
+锘縰sing System;

the funcs in owin are so much smaller these days I'm not sure I see the
point in aliasing them.

It looks a bit nicer with

UseNancy(this Action)

than

UseNancy(this Action<Func<IDictionary<string, object>, Task>, Func<IDictionary<string, object>, Task>>)

[image: 馃槈]


Reply to this email directly or view it on GitHubhttps://github.com//pull/1544/files#r13118572
.

Copy link
Member

@damianh damianh May 28, 2014

Choose a reason for hiding this comment

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

Copy link
Member Author

@khellang khellang May 28, 2014

Choose a reason for hiding this comment

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

I agree it looks fugly, but who cares about the signature anyway? As long as you get the extension methods 馃槃 This problem could probably be solved with your proposed global alias?

Copy link
Member

@damianh damianh May 28, 2014

Choose a reason for hiding this comment

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

Probably not - aliases are only good within a project. AssemblyNeutral delegates would be nice though (if that even makes sense)

Copy link
Member

@damianh damianh Jun 11, 2014

Choose a reason for hiding this comment

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

In the name of consistency, which I value more, +1 for putting them inside the namespace scope. I do this and live with it.

@grumpydev
Copy link
Member

@grumpydev grumpydev commented May 25, 2014

Something is very wrong with this PR.. it's from @khellang and Travis has actually built without errors :shipit:

@jchannon
Copy link
Member

@jchannon jchannon commented May 25, 2014

Its Sunday, Travis takes the day off

@khellang
Copy link
Member Author

@khellang khellang commented May 25, 2014

lol. yeah, crazy! I just popped a 馃嵑 to celebrate! 馃槈

@jchannon
Copy link
Member

@jchannon jchannon commented May 25, 2014

Reckon we could get this in 0.23?

@grumpydev
Copy link
Member

@grumpydev grumpydev commented May 25, 2014

Could do.. tbh the most important part is probably obsoleting the old extension method, although I am still not sure what the message should be.

There is value in having the owin stuff in core though I suppose.

@grumpydev grumpydev added this to the 0.23 milestone May 25, 2014
@jchannon
Copy link
Member

@jchannon jchannon commented May 25, 2014

/// <param name="builder">The application builder delegate.</param>
/// <param name="action">A configuration builder action.</param>
/// <returns>The application builder delegate.</returns>
public static Action<MiddlewareFunc> UseNancy(this Action<MiddlewareFunc> builder, Action<NancyOptions> action)
Copy link
Member

@damianh damianh May 27, 2014

Choose a reason for hiding this comment

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

The suggested builder signature is Action<IDictionary<string, object>, MidFunc> ... which isn't even a proposal yet.

Copy link
Member Author

@khellang khellang May 27, 2014

Choose a reason for hiding this comment

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

Say what? No one's said this was BuildFunc. I assume you're hinting at the name? Do you have a suggestion?

Copy link
Member

@damianh damianh Jun 11, 2014

Choose a reason for hiding this comment

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

Right, ignore moi

@damianh
Copy link
Member

@damianh damianh commented May 28, 2014

Keep Nancy.Owin - IAppBuilder and owin.dll will be around for some time yet.

+1 to moving NancyOwinHost to core. Though I'd rename it to NancyMiddleware myself, because Owin Hosts are things like Nowin, HttpListener etc.

@thecodejunkie thecodejunkie modified the milestones: 0.24, 0.23 Jun 4, 2014
@grumpydev grumpydev modified the milestones: Future, 0.24 Jun 10, 2014
@khellang
Copy link
Member Author

@khellang khellang commented Jun 10, 2014

And NancyMiddleware was born

@damianh damianh modified the milestones: 1.0Alpha, Future Jun 11, 2014
@damianh damianh self-assigned this Jun 11, 2014
@damianh
Copy link
Member

@damianh damianh commented Jun 11, 2014

After we get some consensus on the using declarations, I'll pull it in! (sooo excited, I pee'd a little)

@grumpydev
Copy link
Member

@grumpydev grumpydev commented Jun 11, 2014

Move em inside :)

@khellang
Copy link
Member Author

@khellang khellang commented Jun 11, 2014

Done... mmm, so much better. not 馃槈

@@ -16,7 +16,7 @@
/// <summary>
/// Nancy host for OWIN hosts
Copy link
Member

@jchannon jchannon Jun 11, 2014

Choose a reason for hiding this comment

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

its not a host?? 馃槃

Copy link
Member Author

@khellang khellang Jun 11, 2014

Choose a reason for hiding this comment

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

Happy now? 馃槃

@damianh
Copy link
Member

@damianh damianh commented Jun 11, 2014

wtf travis lol

@grumpydev
Copy link
Member

@grumpydev grumpydev commented Jun 11, 2014

Travis build failed.. be good to see if any @NancyFx/most-valued-minions can restart it.

@grumpydev
Copy link
Member

@grumpydev grumpydev commented Jun 11, 2014

Come on then @NancyFx/most-valued-minions .. who's going to tag this against the milestone and push the magic button? ;)

@jchannon
Copy link
Member

@jchannon jchannon commented Jun 11, 2014

I think @damianh has got his finger on the trigger

@damianh
Copy link
Member

@damianh damianh commented Jun 11, 2014

I am primed.

damianh added a commit that referenced this issue Jun 11, 2014
Move NancyOwinHost to core.
@damianh damianh merged commit b871628 into NancyFx:master Jun 11, 2014
1 check passed
@phillip-haydon
Copy link
Member

@phillip-haydon phillip-haydon commented Jun 11, 2014

We need to roll this back, merge was bad, I didn't approve... Clearly this isn't a team effort...

@damianh
Copy link
Member

@damianh damianh commented Jun 11, 2014

It happened.

Image

@horsdal
Copy link
Member

@horsdal horsdal commented Jun 11, 2014

馃懐 馃嵏

@khellang khellang deleted the owin branch Jun 11, 2014
@grumpydev grumpydev modified the milestones: 1.0 Alpha, 1.0 Feb 4, 2015
@khellang khellang restored the owin branch Feb 20, 2015
@khellang khellang deleted the owin branch Feb 20, 2015
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.

7 participants