adjustments #10

PavelPolyakov · 2015-05-18T16:21:27Z

support reply.redirect() in the next() method
support setting the policies apply points manually, by default they stay onPreHandler and onPostHandler

… points

houndci-bot · 2015-05-18T16:21:40Z

lib/index.js

@@ -80,14 +81,14 @@ var loadPolicies = function(server, policyDirectory, next) {
            data.names.push(match[1]);

            // Add this policy function to the data object
-            var policy = require(path.join(policyDirectory, filename));
+            var policy = require(path.join(options.policyDirectory, filename));


'require' is not defined.

coveralls · 2015-05-18T16:26:01Z

Coverage remained the same at 100.0% when pulling 1da8ed6 on PavelPolyakov:master into 8b91eba on mark-bradshaw:master.

mark-bradshaw · 2015-05-18T20:09:01Z

Thanks @PavelPolyakov. On item #2 You took an interesting approach to being able to switch out the extension points. It's spurred some thoughts for me. One thing I would like to preserve is the option to bind policies to any and all extension points, rather than limited to just two, a "pre" and a "post". I think we need to allow policies to bind to any of the events, so I'll probably pull this PR in but alter it so that policies still need to specify which events to bind to, but extend MrHorse to work with all current Hapi events in the request life cycle.

So, for instance, each policy can indicate that it should bind to onRequest, onPreAuth, onPostAuth, onPreRender, onPostRender, etc. That would allow for maximum flexibility.

PavelPolyakov · 2015-05-18T20:25:04Z

@mark-bradshaw
I think, in general, there is no case when some policy should be applied twice during the one request cycle.
I can't even came with situation, when you should use pre and post at the same time. Could you give an example?

But, to make it more general, I think we can do it in the way when it should be stated in the policy - where to apply it. And default application point should be stated for the whole plugin.

What do you think?

mark-bradshaw · 2015-05-18T20:43:52Z

I had some scenarios int he past, but I can't come up with an example right
off the top of my head. But what I was referring to was that I'd like to
allow MrHorse to be able to have policies that applied to more than just
two events at a time. Currently it can only do policies on prehandler and
posthandler. I'd like to make it so that all events can be used for your
policies. So you might have some policies on preHandler, and some on
preAuth, and others on postHandler, etc. We should set it up to do all
events, and then each policy can just say where it should run.

So to recap, I'm not so concerned with the option to run a single policy on
multiple events, as just making it super easy to use any of the events that
Hapi provides without needing any additional configuration.

I still think that onPreHandler makes the most sense as a default, but I
have no problem with allowing that to be configurable.

On Mon, May 18, 2015 at 3:25 PM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
I think, in general, there is no case when some policy should be applied
twice.
I can't even came with situation, when you should use pre and post at
the same time. Could you give an example?

But, to make it more general, I think we can do it in the way when it
should be stated in the policy - where to apply it. And default application
point should be stated for the whole plugin.

What do you think?

—
Reply to this email directly or view it on GitHub
#10 (comment).

mark-bradshaw · 2015-05-18T20:44:59Z

Also, thanks for the PR. I'd like to see those modifications we've talked about before merging, but I've added you as a collaborator to the repo.

PavelPolyakov · 2015-05-18T20:52:00Z

@mark-bradshaw
ok, I understood you.

I would update it in the way so it's possible to set the application point right in the policy, the default one would be onPreHandler.
However, this update, from my point of view, would in its turn led to the fact that we would not keep current pre and post policy configuration.
And we would have only something like applyPoint configuration option.

mark-bradshaw · 2015-05-18T20:56:59Z

So each policy would just have one event it would bind to, and that is set
in the policy itself. That's fine with me, but I wonder if we can't have
both options just about as easily. We still need to expand MrHorse to know
about the other life cycle events, but the current code is setup to allow
policies to run on multiple events. What if each policy could supply
either a single event, or an array of events to run on. I'm still thinking
of going for maximum flexibility as long as it doesn't become hard to
understand or code.

On Mon, May 18, 2015 at 3:52 PM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
ok, I understood you.

I would update it in the way so it's possible to set the application point
right in the policy, the default one would be onPreHandler.
However, this update, from my point of view, would in its turn led to the
fact that we would not keep current pre and post policy configuration.
And we would have only something like applyPoint configuration option.

—
Reply to this email directly or view it on GitHub
#10 (comment).

PavelPolyakov · 2015-05-18T21:00:22Z

@mark-bradshaw
it's not hard to code, but I can't find any case when you actually need to check the request for one policy during the different events from the lifecycle.

mark-bradshaw · 2015-05-18T21:01:19Z

OK. Works for me. Please make the necessary modifications and then merge
the PR when it looks good.

On Mon, May 18, 2015 at 4:00 PM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
it's not hard to code, but I can't find any case when you actually need to
check the request for one policy during the different events from the
lifecycle.

—
Reply to this email directly or view it on GitHub
#10 (comment).

houndci-bot · 2015-05-19T12:47:03Z

lib/index.js

+_applyPoints.forEach(function (applyPoint) {
+    handlers[applyPoint] = function (request, reply) {
+        runPolicies(data[applyPoint], request, reply);
+    }


Missing semicolon.

coveralls · 2015-05-19T12:49:46Z

Coverage remained the same at 100.0% when pulling 3de1c35 on PavelPolyakov:master into 8b91eba on mark-bradshaw:master.

PavelPolyakov · 2015-05-19T12:51:22Z

@mark-bradshaw
Please, review the current variant, if it's ok - I would update the documentation then.

The concept is like we've discussed.

The default application policy is onPreHandler
One can set the different default application point for the whole plugin, passing the applyPoint option during the initialisation.
One can set the different application point for the particular policy, setting the configuration parameter applyPoint when being in the policy: https://github.com/PavelPolyakov/mrhorse/blob/master/test/policies/secondPasses.js

The list of the possible application points is hardcoded.
https://github.com/PavelPolyakov/mrhorse/blob/master/lib/index.js#L10

Looking forward for the feedback.

mark-bradshaw · 2015-05-19T13:35:25Z

Thanks a ton. I'll review that and get back to you.

On Tue, May 19, 2015 at 7:51 AM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
Please, review the current variant, if it's ok - I would update the
documentation then.

The concept is like we've discussed.

The default application policy is onPreHandler

One can set the different default application point for the whole
plugin, passing the applyPoint option during the initialisation.

One can set the different application point for the particular policy,
setting the configuration parameter applyPoint when being in the policy:
https://github.com/PavelPolyakov/mrhorse/blob/master/test/policies/secondPasses.js

The list of the possible application points is hardcoded.
https://github.com/PavelPolyakov/mrhorse/blob/master/lib/index.js#L10

Looking forward for the feedback.

—
Reply to this email directly or view it on GitHub
#10 (comment).

mark-bradshaw · 2015-05-19T14:00:44Z

Looks good to me. Very well done. We have slightly different code styles, but it's close enough that we probably don't need a style guide for contributions at this moment. In general I try to follow the hapi style guide, but I don't follow it exactly. I should probably making a CONTRIBUTING.md file and mention that.

So, here's some follow on items I can think of:

Add an error warning if you mistype the name of an event
We need to update the README to show the new functionality
We need to add a section for upgrading, since this PR represents a breaking change
You should add a CONTRIBUTORS.md file with your name and contact info, web, whatever so you can get credit for your help, and welcome!
Are there additional tests that we can add to verify the new functionality, such as making sure that you can add to other apply points, or get an error if you mistype an event name?
Should we have some special handling on policy adding to ease the transition from the old style of event binding to the new? if (policy.pre) { // add to onPreHandler }

houndci-bot · 2015-05-19T16:59:06Z

lib/index.js

-                    data.setPreHandler = true;
-                }
+            // Check if the apply point is correct
+            if(policy.applyPoint && _applyPoints.indexOf(policy.applyPoint) === -1) {


Line is too long.
'_applyPoints' is not defined.

houndci-bot · 2015-05-19T17:13:53Z

lib/index.js

-            if (policy.pre === undefined || policy.pre) {
-                server.log(['info'], 'Adding a new PRE policy called ' + match[1]);
-                data.pre[match[1]] = policy;
+            var policy = require(path.join(options.policyDirectory, filename));


'require' is not defined.

coveralls · 2015-05-19T17:39:26Z

Coverage remained the same at 100.0% when pulling eba3d91 on PavelPolyakov:master into 8b91eba on mark-bradshaw:master.

mark-bradshaw · 2015-05-19T20:26:00Z

@PavelPolyakov It looks like you're all done. Looks fine to me. Feel free to merge unless you have more you want to do.

support of the request.reply in the next() support of the different application points for the policies

PavelPolyakov · 2015-05-20T07:21:57Z

@mark-bradshaw
Hi Mark,

I've merged the pull request and created the release.
However, I don't know how to update the package on the npm side.

Is that something you should do? Or I can do it myself somehow.

Just in case, me on the npm: https://www.npmjs.com/~pavel.polyakov

mark-bradshaw · 2015-05-20T15:04:02Z

I'll take care of it. I'm adjusting the README a bit and updating tests.
Since this is a breaking change I'm bumping the version to 1.0.0.

On Wed, May 20, 2015 at 2:21 AM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
Hi Mark,

I've merged the pull request and created the release.
However, I don't know how to update the package on the npm side.

Is that something you should do? Or I can do it myself somehow.

—
Reply to this email directly or view it on GitHub
#10 (comment).

PavelPolyakov · 2015-05-20T15:31:04Z

Thanks,

I'm waiting to include it in the project we're working on.

mark-bradshaw · 2015-05-20T15:40:36Z

OK. I'm waiting for Travis to verify all the tests, and then I'll publish.

On Wed, May 20, 2015 at 10:31 AM Pavel notifications@github.com wrote:

Thanks,

I'm waiting to include it in the project we're working on.

—
Reply to this email directly or view it on GitHub
#10 (comment).

mark-bradshaw · 2015-05-20T16:30:23Z

It's done. I've pushed v1.0.0 to npm. Just as a note, I altered the
options.applyPoint to options.defaultApplyPoint so that it was more clear
what that did.

On Wed, May 20, 2015 at 10:40 AM Mark Bradshaw mark.bradshaw@gmail.com
wrote:

OK. I'm waiting for Travis to verify all the tests, and then I'll publish.

On Wed, May 20, 2015 at 10:31 AM Pavel notifications@github.com wrote:

Thanks,

I'm waiting to include it in the project we're working on.

—
Reply to this email directly or view it on GitHub
#10 (comment)
.

PavelPolyakov · 2015-05-20T17:06:09Z

Should we wait for some time?

Because here:
https://www.npmjs.com/package/mrhorse

I see 0.0.3 still.

mark-bradshaw · 2015-05-20T17:23:55Z

Yes. It'll take a bit of time to show up.

On Wed, May 20, 2015 at 12:06 PM Pavel notifications@github.com wrote:

Should we wait for some time?

Because here:
https://www.npmjs.com/package/mrhorse

I see 0.0.3 still.

—
Reply to this email directly or view it on GitHub
#10 (comment).

PavelPolyakov · 2015-05-21T07:25:51Z

@mark-bradshaw
Are we sure we pushed everything to the npm in the correct way?
Because the number there is still 0.0.3 :)

https://www.npmjs.com/package/mrhorse

mark-bradshaw · 2015-05-21T12:25:05Z

It's odd. I definitely published yesterday. I tried again this morning to
publish, but it said I couldn't overwrite a version already in npm, which
indicates that it does have the new version.

Just for kicks I tried again with a minor version bump, and published again
(npm version minor && npm publish). The site still isn't updating. I'm
not sure what's missing or not.

I've contacted npm support. Hopefully they can clue me in. If you have
any thoughts please let me know.

Mark

On Thu, May 21, 2015 at 2:25 AM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
Are we sure we pushed everything to the npm in the correct way?
Because the number there is still 0.0.3 :)

https://www.npmjs.com/package/mrhorse

—
Reply to this email directly or view it on GitHub
#10 (comment).

mark-bradshaw · 2015-05-21T12:26:50Z

My second try just went through. So apparently it's safer just to use the
npm version command to bump versions. I'll update the build/release
instructions.

On Thu, May 21, 2015 at 7:24 AM Mark Bradshaw mark.bradshaw@gmail.com
wrote:

It's odd. I definitely published yesterday. I tried again this morning
to publish, but it said I couldn't overwrite a version already in npm,
which indicates that it does have the new version.

Just for kicks I tried again with a minor version bump, and published
again (npm version minor && npm publish). The site still isn't updating.
I'm not sure what's missing or not.

I've contacted npm support. Hopefully they can clue me in. If you have
any thoughts please let me know.

Mark

On Thu, May 21, 2015 at 2:25 AM Pavel notifications@github.com wrote:

@mark-bradshaw https://github.com/mark-bradshaw
Are we sure we pushed everything to the npm in the correct way?
Because the number there is still 0.0.3 :)

https://www.npmjs.com/package/mrhorse

—
Reply to this email directly or view it on GitHub
#10 (comment)
.

PavelPolyakov added 3 commits May 18, 2015 18:11

support of the redirect in next; support different policy application…

a0b891a

… points

readme update

734640e

readme update

1da8ed6

houndci-bot reviewed May 18, 2015
View reviewed changes

any application point for any policy

3de1c35

houndci-bot reviewed May 19, 2015
View reviewed changes

updating the README, tests

8d2a7d7

houndci-bot reviewed May 19, 2015
View reviewed changes

fixing a typo

eba3d91

houndci-bot reviewed May 19, 2015
View reviewed changes

PavelPolyakov added a commit that referenced this pull request May 19, 2015

Merge pull request #10 from PavelPolyakov/master

1e00bcf

support of the request.reply in the next() support of the different application points for the policies

PavelPolyakov merged commit 1e00bcf into mark-bradshaw:master May 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adjustments #10

adjustments #10

PavelPolyakov commented May 18, 2015

houndci-bot May 18, 2015

coveralls commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

houndci-bot May 19, 2015

coveralls commented May 19, 2015

PavelPolyakov commented May 19, 2015

mark-bradshaw commented May 19, 2015

mark-bradshaw commented May 19, 2015

houndci-bot May 19, 2015

houndci-bot May 19, 2015

coveralls commented May 19, 2015

mark-bradshaw commented May 19, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 21, 2015

mark-bradshaw commented May 21, 2015

mark-bradshaw commented May 21, 2015

adjustments #10

adjustments #10

Conversation

PavelPolyakov commented May 18, 2015

houndci-bot May 18, 2015

Choose a reason for hiding this comment

coveralls commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

PavelPolyakov commented May 18, 2015

mark-bradshaw commented May 18, 2015

houndci-bot May 19, 2015

Choose a reason for hiding this comment

coveralls commented May 19, 2015

PavelPolyakov commented May 19, 2015

mark-bradshaw commented May 19, 2015

mark-bradshaw commented May 19, 2015

houndci-bot May 19, 2015

Choose a reason for hiding this comment

houndci-bot May 19, 2015

Choose a reason for hiding this comment

coveralls commented May 19, 2015

mark-bradshaw commented May 19, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 20, 2015

mark-bradshaw commented May 20, 2015

PavelPolyakov commented May 21, 2015

mark-bradshaw commented May 21, 2015

mark-bradshaw commented May 21, 2015