search

smart-facility / cognicity-reports-powertrack

cognicity-reports: NodeJS app - Twitter & GNIP PowerTrack support for the CogniCity framework
5 stars 4 forks source link

JSHint issues on master #18

Closed benatwork99 closed 8 years ago

benatwork99 commented 8 years ago

I think we can just set travis to use node 4.

If we do this I think the build failure will disappear as we'll get the newer version of SemVer. See https://github.com/jshint/jshint/issues/2765.

We'll still need to fix the global and globalstrict issue before the next jshint release. But the W020 issue is a bug which will hopefully be fixed before the next proper release - https://github.com/jshint/jshint/issues/2761.

Any objections if I make and commit these changes to master?

matthewberryman commented 8 years ago

I've fixed both of these issues (strict and W020, which I worked around) in 2.0.x —shifting node to v4.2.1 doesn't resolve them but is a good idea and I've done that in 2.0.x already too. There's no point fixing in master because we'll fold 2.0.x including the fixes back into master anyway.

I will look and see if we can't have separate build status icons for branches in READMEs on Monday.

On 27 Nov 2015, at 15:58, Ben Jones notifications@github.com wrote:

I think we can just set travis to use node 4.

If we do this I think the build failure will disappear as we'll get the newer version of SemVer. See jshint/jshint#2765.

We'll still need to fix the global and globalstrict issue. But the W020 issue is a bug which will hopefully be fixed before the next proper release - jshint/jshint#2761.

Any objections if I make and commit these changes to master?

— Reply to this email directly or view it on GitHub.

matthewberryman commented 8 years ago

Also, in future, please keep 2.0.x milestone for things that are specific to 2.0.x

benatwork99 commented 8 years ago

Sorry re 2.0.x tag, of course.

Fair enough re. not fixing master.

FYI, locally, updating node and npm and reinstalling jshint did fix the problem because we stopped getting the RC release when we got the newer semver and got jshint 2.8.0 which tests OK. I assumed npm version of travis was linked to the node version but am not certain of that. Doesn't matter though, as you say we'll wait until we merge.

matthewberryman commented 8 years ago

jshint must have fixed the W020 issue since I took a look (I did an npm install with recent npm). The globalstrict option was deprecated.

Regards, Dr Matthew Berryman

On 27 Nov 2015, at 16:26, Ben Jones notifications@github.com wrote:

Sorry re 2.0.x tag, of course.

Fair enough re. not fixing master.

FYI, locally, updating node and npm and reinstalling jshint did fix the problem because we stopped getting the RC release when we got the newer semver and got jshint 2.8.0 which tests OK. I assumed npm version of travis was linked to the node version but am not certain of that. Doesn't matter though, as you say we'll wait until we merge.

— Reply to this email directly or view it on GitHub.

benatwork99 commented 8 years ago

On npm >= 2 you should get jshint 2.8.0 when you do a 'npm install jshint' or a fresh build, I think?

matthewberryman commented 8 years ago

@benatwork99 now that I've read all the links you sent thoroughly—yes I see now there's a dependency hell issue where lower versions of npm will pick up higher versions of packages if the tag ends in -rc, hence it was picking up a buggy version on travis-ci (locally I use homebrew which has insanely high versions of node and npm). I have now in cognicity-reports-powertrack reverted the workaround with 981379c. I will look at other cognicity-* packages tomorrow and make sure node is up-to-date and revert any other workarounds for the W020 and fix the strict/globalstrict deprecation issue too if not already resolved, except for master branches that will pick up 2.0.x changes.