Skip to content

Conversation

@geoffp
Copy link
Contributor

@geoffp geoffp commented Feb 27, 2016

Addresses #274 .

This PR is for code review as we develop our code formatting standards. We are re-doing white space, fixing errors, and removing superfluous iifes.

I'm reformatting JS files to pass lint as I go, based on my editor's integrated eslint feedback. @bmuenzenmeyer, would you please give the reformatted JS a look and tell me what's in there that hurts your eyes?

@geoffp
Copy link
Contributor Author

geoffp commented Feb 27, 2016

I hate how hard it is to review whitespace-heavy patches.

Here's the secret "diff that ignores whitespace" link:
#275

@bmuenzenmeyer
Copy link
Member

Hey @geoffp I see you've been busy today!

Thanks for taking the lead on this - I am happy that we'll be in a more consistent place once done.

As far as eye bleeding goes, the only thing that stood out to me so far was

 for (i = 0; i < items.indexOf(loopNumberString); i++) {

I am not accustomed to a space after the for - but I can get used to it - leave it this way.

@geoffp
Copy link
Contributor Author

geoffp commented Feb 28, 2016

I wasn't for doing it that way at first, myself, but I was eventually persuaded by somebody or other's best practices this or that. Probably Crockford. I find it's kind of nice to visually distinguish this and if from function calls.

@bmuenzenmeyer
Copy link
Member

pulled this down and running now. There are a LOT of eslint errors. some are good. some seem a bit crazy. will research tweaks and discuss

@bmuenzenmeyer
Copy link
Member

@geoffp I loosened the rules for two lints on c7609d7

"block-scoped-var": 1,
"no-redeclare": 1,

Can you think of a convincing reason not to just turn these off? I feel comfortable with utilizing the conventions eslint is complaining about.

I also shortened the eslint consecutive variable declaration indent - because editorconfig did not seem to support it, so the two tools would create contention. 0714855

"indent": [2, 2, {"SwitchCase": 1, "VariableDeclarator": 1}],

so now we get the following, which I can live with

var viewAllPatterns = [],
  patternPartial = "viewall-" + pattern.patternGroup,
  j;

I don't want to all var's have to be at the beginning of a function, but I wonder if there is a setting to prevent consecutive declarations?

var pattern = patternlab.patterns[i];
var bucketName = pattern.name.replace(/\\/g, '-').split('-')[1];

@bmuenzenmeyer
Copy link
Member

Reassigned you to review what I did

@geoffp
Copy link
Contributor Author

geoffp commented Feb 28, 2016

Yeah, it seems like eslint can get pretty much as bonkers as you want. Definitely not the kind of thing where you'd just enable every rule! I like at least that it gets you thinking about what's okay to do and what's dangerous, even if some of it is pretty paranoid.

No, I'm cool with turning those rules off completely.

On consecutive variable declarations, there's this: http://eslint.org/docs/1.10.3/rules/one-var

@geoffp geoffp assigned bmuenzenmeyer and unassigned geoffp Feb 28, 2016
bmuenzenmeyer pushed a commit that referenced this pull request Feb 29, 2016
@bmuenzenmeyer bmuenzenmeyer merged commit 717e139 into dev Feb 29, 2016
@bmuenzenmeyer bmuenzenmeyer deleted the code-cleanup branch June 3, 2016 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants