Skip to content

Linting updates#211

Merged
lemonmade merged 3 commits intomasterfrom
linting-updates
Aug 19, 2016
Merged

Linting updates#211
lemonmade merged 3 commits intomasterfrom
linting-updates

Conversation

@lemonmade
Copy link
Member

This PR makes a few small updates to our linting config:

  • Updated the indent rule to force case statements and member expressions nested on multiple lines to be indented one level (2 spaces). Currently, case is not supposed to be indented, and we have no lint rule that dictates multiline member expressions.
  • Updated the type-id-match rule for flow to require all flow types to match <Something>(Type|Props|State|Context), which I found to be a good setting to use for React stuff.
  • Updated all the dependencies and configured the new rules (most are just turned off). The biggest addition is a rule that prevents using interpolation-like sequences in strings that aren't template literals (i.e., no '${foo}', since this is probably an error).

cc/ @Shopify/javascript

'id-match': 'off',
// This option sets a specific tab width for your code
'indent': ['warn', 2],
'indent': ['warn', 2, {SwitchCase: 1, MemberExpression: 1}],
Copy link

Choose a reason for hiding this comment

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

😍

Choose a reason for hiding this comment

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

👍

@bouk
Copy link
Contributor

bouk commented Aug 17, 2016

I disagree with indenting case statements. Unindented is the standard style in every other language, so I don't see why we should go against that

@harisaurus
Copy link

harisaurus commented Aug 17, 2016

  switch (action.type) {
  case types.UPDATE_TEXT: {
    const componentText = Object.keys(action.text).reduce((components, componentKey) => {
      components[componentKey] = _.merge({}, state[componentKey], action.text[componentKey]);
      return components;
    }, {});

    return Object.assign({}, state, componentText);
  }
  default: {
    return state;
  }
  }

you have to admit that this looks very strange. Text editors when set to JS also indent case statements by default, so there's some differences set for JS vs other languages

@bouk
Copy link
Contributor

bouk commented Aug 17, 2016

It looks weird because you're putting brackets around case statements, which you don't need to (I've never seen that done before?)

@harisaurus
Copy link

We enforce the brackets to ensure lexical declarations only apply to a particular case: http://eslint.org/docs/rules/no-case-declarations

@bouk
Copy link
Contributor

bouk commented Aug 19, 2016

whatever, I don't care enough about the case thing, :shipit:

@lemonmade lemonmade merged commit 35e3b31 into master Aug 19, 2016
@lemonmade lemonmade deleted the linting-updates branch August 19, 2016 18:21
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.

4 participants