Skip to content
This repository has been archived by the owner on Dec 5, 2018. It is now read-only.

chore(build): Initial stab at Grunt skeleton #32

Merged
merged 4 commits into from
Dec 17, 2014

Conversation

pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Dec 17, 2014

A case study in how not to do pull requests...

But we should now have a shared precommit-hook which runs lint tasks whenever you try and commit some code.
JSHint and JSCS rules are up for negotiation.

FIxes #31
Fixes #17
FIxes #13
Fixes #12
Fixes #11

@pdehaan
Copy link
Contributor Author

pdehaan commented Dec 17, 2014

One PR to rule them all, One PR to find them,
One PR to bring them all and in the darkness bind them
In the Land of GitHub where the Shadows lie.

@jaredhirsch
Copy link
Member

See my comments in #31 about the jshint / jscsrc rules.

@jaredhirsch
Copy link
Member

That changelog script is really awesome, and makes me much happier about the angular-style commit conventions!

Nick Chapman <nchapman@users.noreply.github.com>
Peter deHaan <peter@deseloper.com>
Vlad Filippov <vlad.filippov@gmail.com>
johngruen <john.gruen@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

@johngruen is this how you want to be remembered by history? If not, maybe hack on your .gitconfig a bit. The user part of mine looks like this:

[user]
    name = Jared Hirsch
    email = ohai@6a68.net

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey Jared, I did locally, but haven't made a PR since.

Copy link
Member

Choose a reason for hiding this comment

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

word.

@@ -19,6 +21,6 @@ server.route({
}
}
});
server.start(function(request, reply) {
server.start(function () {
Copy link
Member

Choose a reason for hiding this comment

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

ehhhhhhh, I dunno. how about inserting /*jshint unused:true */ instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

afaik, the server start() method doesn't take parameters, and only routes have request and reply params, not the server start() itself.

http://hapijs.com/api#serverstartcallback

Correction. It looks like server.start() takes a single callback as a paramter with the following signature function (err) {...}. So we may want to add a check for err being non-null and pipe that through heka or something something.

Copy link
Member

Choose a reason for hiding this comment

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

oh my. let's just leave it as is with your commit, then

Copy link
Member

Choose a reason for hiding this comment

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

(I'll make sure the server.start actually logs once the server actually does stuff ^_^)

@jaredhirsch
Copy link
Member

ok, looks good to me (modulo concerns I've called out above). feel free to merge once you've addressed my concerns; nchapman can file a followup for any jshint or jscs rules he'd like to change 🍻

@pdehaan pdehaan assigned pdehaan and nchapman and unassigned pdehaan Dec 17, 2014
@pdehaan
Copy link
Contributor Author

pdehaan commented Dec 17, 2014

To @nchapman for his eyes and green button pressing skills.

@nchapman
Copy link
Contributor

This looks great. Nicely done @pdehaan!

nchapman added a commit that referenced this pull request Dec 17, 2014
chore(build): Initial stab at Grunt skeleton
@nchapman nchapman merged commit 1a849d5 into mozilla:master Dec 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants