Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Nancy module split #906

Merged
merged 11 commits into from
Jan 25, 2013
Merged

Nancy module split #906

merged 11 commits into from
Jan 25, 2013

Conversation

grumpydev
Copy link
Member

Splits NancyModule into INancyModule and NancyModule, with the framework only relying on the properties present in the interface.

Also added a sample of a new nancy module type, which uses attributes to define routes, to show the flexibility this brings.

@thecodejunkie
Copy link
Member

I'm guessing it is because of R# and not your doing, but there are a ton of changes in this PR that are just added this. when accessing either Context or Response in modules etc. Maybe we should back those out, they make up a substantial part of the number of modified files? Scanning over it, and it looks like it's 2/3rd of the changes, give or take a few

using Nancy.ViewEngines;

/// <summary>
/// Nancy base class
Copy link
Member

Choose a reason for hiding this comment

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

It's not a base class so I would change it into something like "Defines the core functionality of a module in Nancy"

@thecodejunkie
Copy link
Member

Other than the review comments it looks good :-)

The idea being that the "core" functionality only relies on
the base, with the minimal amount of properties on it, leaving
the end users free to implement their own route builders etc.
Obviously it wasn't going to work in the builder as
you add to the pipelines in the module ctor :P
It was re-running the select statement and makign the
assumption that would return the same instances as the
original - which in this case it wasn't
grumpydev added a commit that referenced this pull request Jan 25, 2013
@grumpydev grumpydev merged commit 21dacb4 into NancyFx:master Jan 25, 2013
@grumpydev grumpydev deleted the NancyModuleSplit branch January 25, 2013 08:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants