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 node dependencies to latest. Fix styling issues #1233

Closed
wants to merge 2 commits into from

Conversation

mtraynham
Copy link
Contributor

Update the node dependencies to latest. Fix any style issues with JSCS/JSHint bump.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 2, 2016

Thanks @mtraynham! This is very helpful.

I hope the churn from jscs eventually settles down. It creates a lot of noise in the commit history. On the plus side, I'm getting really good at git blame sha^ -- foo.js 😉

@mtraynham
Copy link
Contributor Author

mtraynham commented Dec 2, 2016

Ha. Well, JSCS has now been deprecated in favor of ESLint, and the contributors aren't adding new features. So I don't think there will be many more iterations of this 😜

@gordonwoodhull
Copy link
Contributor

Uh oh, I bet ESLint will complain about a subtly very different set of style quirks. 😱

@gordonwoodhull
Copy link
Contributor

Thanks @mtraynham, merged for 2.0!

@mtraynham mtraynham deleted the update_node_deps branch December 5, 2016 18:24
gordonwoodhull added a commit that referenced this pull request Dec 9, 2016
was getting:
Warning: Path must be a string. Received undefined

i can't find any reference to the `basepath` we were using before, guess
that was undocumented stuff that got removed. `cwd` does the same thing a
different way

ref: #1233
@ialarmedalien
Copy link
Contributor

FYI: I converted the current JSCS and JSHint configs into ESLint using polyjuice, and then linted the current master branch. Some of the results can be fixed automatically, and some could probably be ignored. I dc-ified the results (because code linting is much more fun when there are graphs to look at!) and included the eslint config that polyjuice generated.

@mtraynham
Copy link
Contributor Author

mtraynham commented Nov 6, 2017

@ialarmedalien this is really impressive 👍 That row chart scrolling solution is pretty clever, haha. Very cool.

Gotta love code hinting, definitely exposes problems. Just my 2 cents, I've been using airbnb's config. It's fairly strict and I think it does a good job overall with some slight modifications.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Nov 6, 2017

That is very cool. Yeah, really neat way to do a scrolling row chart! A lot of people ask how to do that.

In terms of the content of the errors, I'd mostly be concerned about the variable shadowing and the jsdoc errors - the rest are stylistic. I guess unused variables could indicate a bug too. We've historically had lighter restrictions on the specs, and it's probably not worth worrying about variable shadowing there, because specs are meant to be copied and pasted.

I'd definitely accept a PR with an upgrade to eslint. I would want to coordinate, though, since this is likely to touch every line in the source, and I'm planning the d3v4 upgrade for EOY when things get quiet at work (which will also touch every line, making a merge painful).

@gordonwoodhull
Copy link
Contributor

finally moved to eslint - on develop for 3.1

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.

3 participants