Make the tests work again

[profOnno]

This will also work with node v4. But then zombie has to be version 4. Travis-ci won't work (yet) with node 4. (building native extensions requires C++11-compatible compiler), maybe with the newer docker travis-ci.

[cwebber]

Wow, incredible! This looks good to merge to me. I'm going to verify on IRC that nobody wants to jump on a more informed review than my quick scan and then merge it in.

[profOnno]

Check the package.json
I did make a update for databank-connect (uses redis-connect as example).
It is pulled straight from my github.

I did forget to use my simplesmtp from github (no tar.gz no the source like i did with databank-connect (i'm learning on the way)). The simplesmtp is deprecated on npm, but I don't see a problem with using it, it's only used in the test code and not on production.

edit:
It seems the change in package.json for the simplesmtp should/can be reversed if this is merged.

[cwebber]

@profOnno Okay, I don't know anything about the databank-connect stuff. Could you ping @evanp about it? I'm happy to merge this otherwise.

[harrisbinkhurram]

hey the new pump.io doesn't seem to be working on ubuntu, after installing dependencies im stuck with use of const in strict mode

any leads?

[profOnno]

@harrisbinkhurram: It will be 'fixed' in this pull request. You can get the patch here https://github.com/profOnno/pump.io/tree/r4p/patch. Only connect.patch (and connect_update.sh) matters. The connect_update.sh will apply the patch and rebuild connect without the latest qs.

[strugee]

@profOnno this looks really, really good! The vast majority of my comments deal with whitespace and stuff like that, and are extremely trivial to fix. Great work!

That being said, shipping a patch doesn't seem like the best approach. Instead, can you send a PR to upstream connect-databank? Then it'll be fixed for everybody and the Pump.io usage won't be so hacky.

[strugee]

I'm also uncomfortable having the package.json refer to GitHub forks. Can we solve stuff like that some other way, e.g. by upgrading the relevant libraries?

[profOnno]

Thanks strugee,

I know the patch-thing is not the best thing to do. It's a kind of a place holder. If the tests work, developers can make other adjustments and see the test break or not break depending on their code. If I'm not mistaking Joyent uses it for this kind of problems. The patch for the connect qs story, will resolve itself when changes are in place to upgrade connect to version 2.x if I'm not mistaken (I've seen the package.json qs version capped).

I can/will put a pull request in for the connect-databank. It is a big rewrite combining the connect-redis (mentioned in the connect documentation as example) and the 'old' connect-databank. I can go back a little and just use the patch small change for this PR (I know this is not favoured, but can be used for place holder). The patch will add the stop method to stop the timers on closing the app, needed for vows to end when done. And keep the connect-databank upgrade for a next round.

For the whitespace comment stuff, i looked at jscs.js (http://jscs.info). So the style can be forced automatic. Helps me to be more style-full and helps you writing less style related comments :p. Please give me your opinion on this (see #1137). Should we have Evans coding style along with the other pre-sets? (Jquery, Crockford, Airbnb, Google, Grunt, Idiomatic, etc..) or should we conform to one of the presets. If you want respond, issue #1137. Nice vid from Douglas Crockford about this https://www.youtube.com/watch?v=_EANG8ZZbRs .

I'll fix the style comments asap, and replace this pull request with a new 'cleaned up' one (with squashed commits, I just figured out a way to do that).

[strugee]

@profOnno I wouldn't spend a lot of effort on connect-databank. If we eventually move to Express 4.x, we'll probably be ditching Connect anyway. So if I were you, I'd send the bare minimum needed to get tests passing.

Also, don't send a new PR. If you push to the same branch, GitHub will automatically update the commits in this PR. (You may have to force-push with -f, but that's okay.)

[strugee]

function() { not function(){

[strugee]

Again, is this actually used?

[profOnno]

In the last function as callback
br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step

[ghost]

Please can you un-subscribe my email address shiju.varghese@dekhdekh.com mailto:shiju.varghese@dekhdekh.com so that I do not receive emails like the one below?

Many thanks.

On 10 Jan 2016, at 10:42, Menno Vossen notifications@github.com wrote:

In test/oauth-authorization-test.js #1136 (comment):

                 },
                 function(err, br) {

•                    var self = this;
  

  In the last function as callback
  br.visit("bla",this)... calls next function in Step. self(!br.success, br); calls outer Step

—
Reply to this email directly or view it on GitHub https://github.com/e14n/pump.io/pull/1136/files#r49274288.

[profOnno]

@ShijuDekhDekh1, I think you have to do that yourself. If you are logged in in github, clicked the link in the email (view it on github) to the origin of the notification. Then you should be on the github page where the notification came from. On the right there should be a button Unsubscribe.

[strugee]

@profOnno whoops, went through the updated code but forgot to comment! All the changes you've made look awesome; I only see a couple things left - like this and the .fill() stuff (there isn't really a precedent in the codebase, AFAICT, but virtually every other JS project I've seen uses the style I've suggested).

Based on the Travis log it looks to me like the removal of SIGKILL is exposing another failure in the test suite, so we should maybe take a look at that

Lastly, we still shouldn't ship patches in the repo. Here's what I propose: if you remove the connect-databank patch and related scripts from this PR, then I'll go ahead and merge it. I know that means the test suite will still fail, but that way we don't have to worry about the rest of this PR sitting untouched while upstream connect-databank gets fixed. Does that sound OK?

[profOnno]

Ok, for the indent part I'll go with your opinion (will do the alignment manual now.. I think it can be done in vim, but couldn't find how at the moment 🐒 ).

this. You are right.. The closing ')' of Step.

I'll add SIGKILL and do a rebase only the last two commits (clean-up, so there is no remove SIGKILL and add SIGKILL, and multiple clean-up commits).

The connect-databank patch can be removed because I'll use the connect-databank from my github (I need the changes... test will fail without). The other patch needs to remain (check the script to see what it does). Newer versions of connect (not yet compatible) have capped the qs version, like i did in the patch.

To be clear. There were 2 patches, one for connect-databank to stop the timers, and one for connect to use an older version of qs.

I'll do the rebase (commit cleanup merges) later.

[profOnno]

strugee... If you comment on the PR and not on the commits I can merge of the last commits to some grouped ones... so change the history to a cleaner history. Starting from this commit.

[strugee]

@profOnno GitHub should preserve comments on the diff even if you push a squashed commit. Go right ahead.

[strugee]

@profOnno with the exception of the patches, which seem really hacky, this looks good to go for me. Nice work!

I asked @cwebber on IRC if we can merge the patch stuff in anyway, with the assumption that it'll go away soonish. We'll see what he says.

[strugee]

@profOnno so, @cwebber says that he's fine with the connect-databank patch going in (i.e. the package.json entry pointing to your GitHub), but not the Connect patch (i.e. the stuff in patch/).

Can you remove the Connect patch from this PR, so I can merge it? I understand that the tests will continue to fail with this change, but we can upgrade Connect in a followup PR, and in the meantime the vast majority of the testsuite failures will be fixed.

[profOnno]

Should be ok now, with no patch folder and a patched connect fork on github. Thought the git rebase -i, to clean only works local?

[strugee]

@profOnno awesome!

After you do git rebase -i you have to force-push to GitHub again.

[strugee]

@profOnno thanks so much for all the work you've done here! It's fantastic.

[profOnno]

I like it. Now only get my pump back up. I think I have a leveldb problem or something, hope I have some time to get it up again, maybe switch to my new CoreOS server and use my pump.ninja domain.

The red crosses next to the commits are from the CLAhub.. How can we get rid of them? (The CLA was removed in favour of DCO)