Skip to content

Commit

Permalink
fix(app-jshint): improve jshint usage
Browse files Browse the repository at this point in the history
Changes:
- Update jshint task in `Gruntfile.js` to include `serverTest`
- Add `server/.jshintrc-spec` that extends `server/.jshintrc` with spec globals
- Use `"latedef": "nofunc"` instead of `"latedef": true` in `server/.jshintrc`
- Add assertion for `jshint` task in generator tests for `defaultOptions`
- Fix pre exsisting lint errors in `server` and `client`
- Change `getEmail()` in `client/app/account/settings/settings.controller` to use `user` arg and not `$scope.user`

Closes #463, #486
  • Loading branch information
kingcody committed Aug 25, 2014
1 parent c89364c commit 35fcf49
Show file tree
Hide file tree
Showing 14 changed files with 73 additions and 44 deletions.
11 changes: 10 additions & 1 deletion app/templates/Gruntfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,16 @@ module.exports = function (grunt) {
options: {
jshintrc: 'server/.jshintrc'
},
src: [ 'server/{,*/}*.js']
src: [
'server/**/*.js',
'!server/**/*.spec.js'
]
},
serverTest: {
options: {
jshintrc: 'server/.jshintrc-spec'

This comment has been minimized.

Copy link
@kingcody

kingcody Mar 6, 2015

Author Member

@villesau this is where we use the .jshintrc-spec file. It was made for use with the jshint:serverTest task. Feel free to rename if you'd like to use it with another build system.

This comment has been minimized.

Copy link
@villesau

villesau Mar 6, 2015

@kingcody Aha! Cool, thanks for the clarification:) Is there some specific reason why you keep the spec-files along with sources? As far as i know, there is no any other way to use separate .jshintrc options with different files than place them in different folders. Now, if you wan't to check your code quality with for example standalone jshint, you have to specify it to use either .jshint-spec or .jshint file which is not good: If you specify it to use first one, you allow all the globals from tests, but if you specify the later one, you will get errors from tests. Either is not good, and for example simple pre-commit/push hooks will fail on this.

},
src: ['server/**/*.spec.js']
},
all: [
'<%%= yeoman.client %>/{app,components}/**/*.js',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ angular.module '<%= scriptAppName %>'
$scope.email = {}

getEmail = (user) ->
return [null, null] unless $scope.user.credentials.length
return [null, null] unless user.credentials.length

for c in $scope.user.credentials when c.type is 'email'
for c in user.credentials when c.type is 'email'
return [c.value, c.confirmed]

[null, null]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@ angular.module('<%= scriptAppName %>')
$scope.email = {};

var getEmail = function(user) {
if (!$scope.user.credentials.length) {
if (!user.credentials.length) {
return null;
}

for(var i in $scope.user.credentials) {
var c = $scope.user.credentials[i];
if(c.type==='email') return [c.value, c.confirmed];
for(var i in user.credentials) {
var c = user.credentials[i];
if (c.type === 'email') {
return [c.value, c.confirmed];
}
}
};

Expand All @@ -36,7 +38,7 @@ angular.module('<%= scriptAppName %>')
$scope.message = '';
});
}
}
};

$scope.changePassword = function() {
$scope.pwd.submitted = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ angular.module('<%= scriptAppName %>')

$scope.delete = <% if(filters.uibootstrap) { %>Modal.confirm.delete(<% } %>function(user) {
User.remove({ id: user._id });
_.remove($scope.users, user)
_.remove($scope.users, user);
}<% if(filters.uibootstrap) { %>)<% } %>;
});
4 changes: 2 additions & 2 deletions app/templates/client/app/main/main.controller(coffee).coffee
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict'

angular.module '<%= scriptAppName %>'
.controller 'MainCtrl', ($scope, $http<% if(filters.socketio) { %>, socket<% } %><% if(filters.uibootstrap && filters.mongoose) { %>, Modal<% } %>) ->
.controller 'MainCtrl', ($scope, $http<% if(filters.socketio) { %>, socket<% } %>) ->
$scope.awesomeThings = []

$http.get('/api/things').success (awesomeThings) ->
Expand All @@ -19,4 +19,4 @@ angular.module '<%= scriptAppName %>'
$http.delete '/api/things/' + thing._id<% } %><% if(filters.socketio) { %>

$scope.$on '$destroy', ->
socket.unsyncUpdates 'thing'<% } %>
socket.unsyncUpdates 'thing'<% } %>
4 changes: 2 additions & 2 deletions app/templates/client/app/main/main.controller(js).js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict';

angular.module('<%= scriptAppName %>')
.controller('MainCtrl', function ($scope, $http<% if(filters.socketio) { %>, socket<% } %><% if(filters.uibootstrap && filters.mongoose) { %>, Modal<% } %>) {
.controller('MainCtrl', function ($scope, $http<% if(filters.socketio) { %>, socket<% } %>) {
$scope.awesomeThings = [];

$http.get('/api/things').success(function(awesomeThings) {
Expand All @@ -24,4 +24,4 @@ angular.module('<%= scriptAppName %>')
$scope.$on('$destroy', function () {
socket.unsyncUpdates('thing');
});<% } %>
});
});
4 changes: 2 additions & 2 deletions app/templates/server/.jshintrc
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
"bitwise": true,
"eqeqeq": true,
"immed": true,
"latedef": true,
"latedef": "nofunc",
"newcap": true,
"noarg": true,
"regexp": true,
"undef": true,
"smarttabs": true,
"asi": true,
"debug": true
}
}
11 changes: 11 additions & 0 deletions app/templates/server/.jshintrc-spec
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"extends": ".jshintrc",
"globals": {
"describe": true,
"it": true,
"before": true,
"beforeEach": true,
"after": true,
"afterEach": true
}
}

This comment has been minimized.

Copy link
@villesau

villesau Mar 5, 2015

Hi! How do you utilize this file? jshint seems not to understand .jshintrc-spec file extension (unless you point it explicitly) and reports all the globals in spec-files. Intellij doesn't get this file either because it does not follow regular jshint file extension convention which makes this type of .jshintrc file really problematic.

2 changes: 1 addition & 1 deletion app/templates/server/api/thing/thing.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ describe('GET /api/things', function() {
done();
});
});
});
});
20 changes: 10 additions & 10 deletions app/templates/server/api/user(auth)/user.model.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ UserSchema
// returns only first found email
// TODO: in case of multiple emails, should prioritize confirmed ones
return this.credentials.filter(function(c) {
return c.type==='email';
return c.type === 'email';

})[0].value;
});
Expand All @@ -90,21 +90,21 @@ UserSchema
.virtual('emails')
.get(function() {
return this.credentials
.filter(function(c) { return c.type==='email'; })
.filter(function(c) { return c.type === 'email'; })
.map(function(c) { return c.value; });

});

UserSchema
.pre('save', function(next) {<% if (filters.oauth) { %>
if(!this.localEnabled) {
if (Object.keys(this.strategies).length===0) {
if (Object.keys(this.strategies).length === 0) {
return next(new Error('No connected accounts'));
}
return next();
}<% } %>

mongoose.models['User']<% if (filters.oauth) { %>
mongoose.models.User<% if (filters.oauth) { %>
.find({ localEnabled:true })<% } %>
.where('credentials.type').equals('email')
.where('credentials.value').equals(this.email)
Expand Down Expand Up @@ -132,15 +132,15 @@ UserSchema.methods = {
},
confirm: function(emailOrPhone, cb) {
this.credentials.forEach(function(c) {
if (c.value===emailOrPhone) {
if (c.value === emailOrPhone) {
c.confirmed = true;
}
});
this.save(cb);
},
changeEmail: function(oldEmail, newEmail, cb) {
this.credentials.forEach(function(c) {
if (c.value===oldEmail) {
if (c.value === oldEmail) {
c.value = newEmail;
c.confirmed = false;
}
Expand Down Expand Up @@ -177,14 +177,14 @@ UserSchema.statics = {
var dataFormatted;
dataFormatted = [];

if (data.email != null) {
if (data.email !== null) {
dataFormatted.push({
'credentials.type': 'email',
'credentials.value': data.email
});
}

if (data.phone != null) {
if (data.phone !== null) {
dataFormatted.push({
'credentials.type': 'phone',
'credentials.value': data.phone
Expand All @@ -195,11 +195,11 @@ UserSchema.statics = {
.or(dataFormatted)
.exec(function(err, users) {
if (err) return cb(err);
if (users.length===0) return cb(null, null);
if (users.length === 0) return cb(null, null);

cb(null, users);
});
}<% } %>
};

module.exports = mongoose.model('User', UserSchema);
module.exports = mongoose.model('User', UserSchema);
6 changes: 3 additions & 3 deletions app/templates/server/api/user(auth)/user.model.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,10 @@ describe('User Model', function() {
});

it("should authenticate user if password is valid", function() {
user.authenticate('password').should.be.true;
return user.authenticate('password').should.be.true;
});

it("should not authenticate user if password is invalid", function() {
user.authenticate('blah').should.not.be.true;
return user.authenticate('blah').should.not.be.true;
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ exports.setup = function (User, config) {
}, function (err, users) {
if (err) return done(err);
if (users) {
var user = users[0];
user = users[0];
user.absorb(profile.provider, profile);

// we can do that because we have it handled by Facebook
user.confirm(profile.emails[0].value);
return done(null, user);
}

var user = new User({
user = new User({
name: profile.displayName,
email: profile.emails[0].value,
username: profile.username,
Expand All @@ -46,4 +46,4 @@ exports.setup = function (User, config) {
});
}
));
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ exports.setup = function (User, config) {
}, function (err, users) {
if (err) return done(err);
if (users) {
var user = users[0];
user = users[0];
user.absorb(profile.provider, profile);

// we can do that because we have it handled by Google
user.confirm(profile.emails[0].value);
return done(null, user);
}

var user = new User({
user = new User({
name: profile.displayName,
email: profile.emails[0].value,
username: profile.username,
Expand All @@ -46,4 +46,4 @@ exports.setup = function (User, config) {
});
}
));
};
};
25 changes: 16 additions & 9 deletions test/test-file-creation.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ describe('angular-fullstack generator', function () {
});

describe('running app', function() {
;

beforeEach(function() {
this.timeout(20000);
fs.mkdirSync(__dirname + '/temp/client');
Expand All @@ -77,6 +77,16 @@ describe('angular-fullstack generator', function () {
});
});

it('should pass jshint', function(done) {
this.timeout(60000);
gen.run({}, function () {
exec('grunt jshint', function (error, stdout, stderr) {
expect(stdout).to.contain('Done, without errors.');
done();
});
});
});

it('should run server tests successfully', function(done) {
this.timeout(60000);
gen.run({}, function () {
Expand Down Expand Up @@ -161,8 +171,7 @@ describe('angular-fullstack generator', function () {
this.timeout(60000);
gen.run({}, function () {
exec('grunt jshint', function (error, stdout, stderr) {
expect(stdout).to.contain('Running "jshint:server" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Running "jshint:all" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Done, without errors.');
done();
});
});
Expand Down Expand Up @@ -203,13 +212,12 @@ describe('angular-fullstack generator', function () {
});
});
});

it('should pass jshint', function(done) {
this.timeout(60000);
gen.run({}, function () {
exec('grunt jshint', function (error, stdout, stderr) {
expect(stdout).to.contain('Running "jshint:server" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Running "jshint:all" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Done, without errors.');
done();
});
});
Expand Down Expand Up @@ -255,8 +263,7 @@ describe('angular-fullstack generator', function () {
this.timeout(60000);
gen.run({}, function () {
exec('grunt jshint', function (error, stdout, stderr) {
expect(stdout).to.contain('Running "jshint:server" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Running "jshint:all" (jshint) task\u001b[24m\n\n✔ No problems');
expect(stdout).to.contain('Done, without errors.');
done();
});
});
Expand Down Expand Up @@ -299,4 +306,4 @@ describe('angular-fullstack generator', function () {
});
});
});
});
});

0 comments on commit 35fcf49

Please sign in to comment.