Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependencies to latest, fix spec linting errors #975

Merged
merged 4 commits into from
Aug 25, 2015

Conversation

mtraynham
Copy link
Contributor

Updates all npm dependencies to latest. Update jscs & jshint options.

@@ -1,28 +1,68 @@
{
"browser": true,
"bitwise": false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, we do have bitwise operators in portions of the code. Turn this off to find them...

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. all of those look legitimate except for this one:

   src/data-table.js
    106 |            bAllFunctions = bAllFunctions & (typeof f === 'function');

which i'll fix when i merge this for the next beta release. it's harmless, but one still shouldn't use bitwise for booleans.

then there's the annoying but apparently very fast n | 0...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

http://jsperf.com/floor-vs-parseint-vs-bitwise

25% faster than Math.floor, but likely negligible performance hit.

Copy link
Contributor

Choose a reason for hiding this comment

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

you're right, it's probably not helping anything. our bottleneck is the DOM, not math.

@mtraynham
Copy link
Contributor Author

Not sure what your preference is on "else on separate lines"

if (...) {
} else {
}

vs

if (..) {
}
else {
}

but I corrected some of those cases.

@mtraynham
Copy link
Contributor Author

One last thing, I have to recommend this global npm dependency, because it works very well.

https://www.npmjs.com/package/npm-check-updates

At the CLI:
npm-check-updates or ncu lists all upgradable packages
ncu -u updates the package.json
Then call npm update to get the new depedencies

@mtraynham
Copy link
Contributor Author

Alrighty, I fixed almost all of the linter errors in ./spec. That was achieved with the new jscs CLI options jscs --fix spec/**.js. It isn't hooked up in the Gruntfile though.

@mtraynham mtraynham changed the title Update dependencies to latest Update dependencies to latest, fix spec linting error Aug 7, 2015
@mtraynham mtraynham changed the title Update dependencies to latest, fix spec linting error Update dependencies to latest, fix spec linting errors Aug 7, 2015
@gordonwoodhull
Copy link
Contributor

Neat, thanks! I'm not crazy about } else { but I can live with it.

@mtraynham
Copy link
Contributor Author

Last update fixes the remaining issues with the spec directory and adds /spec to the jscs/jshint paths to lint. I want to point out as well, we have a script at ./spec/helpers/jasmine-jsreporter.js. It is a duplicated copy of the file at https://github.com/detro/jasmine-jsreporter/blob/master/jasmine-jsreporter.js, but it's out of date.

I'd like to get this removed from the source and retrieved using npm instead.

@gordonwoodhull
Copy link
Contributor

Can't find an npm module for that - is there a standard, safe way to pull sources from github?

It seems to be used by grunt-saucelabs, which just says pull it from github.

https://www.npmjs.com/package/grunt-saucelabs#test-result-details-with-jasmine

@gordonwoodhull
Copy link
Contributor

Actually, there seem to be two copies within grunt-saucelabs itself:

-rw-r--r--  1 gordonwoodhull  staff  12355 May 30  2014 node_modules/grunt-saucelabs//examples/jasmine/lib/jasmine-jsreporter/jasmine-jsreporter.js
-rwxr-xr-x  1 gordonwoodhull  staff   6468 Jan  3  2014 node_modules/grunt-saucelabs//test/jasmine/lib/jasmine-jsreporter.js

@mtraynham
Copy link
Contributor Author

Ya know for the life of me, I can't figure out how to install it directly from github... I for certain have done this before. I'll create a PR on that repo for a package.json and a request publish to npm...

@gordonwoodhull
Copy link
Contributor

I think it's probably fine to use the copy from saucelabs, since that's what we're using it for.

@mtraynham
Copy link
Contributor Author

sounds good to me

@gordonwoodhull gordonwoodhull mentioned this pull request Aug 24, 2015
@gordonwoodhull gordonwoodhull merged commit 43ba74c into dc-js:master Aug 25, 2015
@mtraynham mtraynham deleted the update_dependencies branch August 26, 2015 20:46
@gordonwoodhull gordonwoodhull mentioned this pull request Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants