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

refactor: adhere mostly to StandardJS guidelines #1657

Merged
merged 1 commit into from
Aug 3, 2017
Merged

Conversation

jviotti
Copy link
Contributor

@jviotti jviotti commented Aug 2, 2017

This commit changes the whole codebase to adhere to all StandardJS
guidelines rules except semicolons, since the removal of semicolons
affect pretty much all lines, and the final diff is very hard to follow
(and to assess other more involved changes).

In a nutshell:

  • When using function, we now require a space before the opening
    parenthesis
  • If a line with operators is broken into multiple lines, the operator
    should now go after the line break
  • Unnecessary padding lines are now forbidden

There were also some minor things that the standard CLI caught that I
updated here.

See: https://standardjs.com
Signed-off-by: Juan Cruz Viotti jv@jviotti.com

This commit changes the whole codebase to adhere to all StandardJS
guidelines rules except semicolons, since the removal of semicolons
affect pretty much all lines, and the final diff is very hard to follow
(and to assess other more involved changes).

In a nutshell:

- When using `function`, we now require a space before the opening
  parenthesis
- If a line with operators is broken into multiple lines, the operator
  should now go after the line break
- Unnecessary padding lines are now forbidden

There were also some minor things that the `standard` CLI caught that I
updated here.

See: https://standardjs.com
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@jhermsmeier
Copy link
Contributor

What do you think about using standard/eslint-config-standard and extend that, to shrink down our custom defined rule set?

@jviotti
Copy link
Contributor Author

jviotti commented Aug 3, 2017

Yep, we definitely should. I'll handle it it another PR.

@jviotti jviotti merged commit 5c19b70 into master Aug 3, 2017
@jviotti jviotti deleted the standardjs-wip branch August 3, 2017 10:59
jviotti added a commit that referenced this pull request Aug 3, 2017
- Extend the `standard` ESLint configuration
- Remove ESLint rules that are defined in the `standard` configuration
- Get rid of semi-colons

See: #1657
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
jviotti added a commit that referenced this pull request Aug 3, 2017
- Extend the `standard` ESLint configuration
- Remove ESLint rules that are defined in the `standard` configuration
- Get rid of semi-colons

See: #1657
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
@lurch
Copy link
Contributor

lurch commented Aug 3, 2017

Out of curiosity, have any / many other resin.io repos switched to this new style?

@jviotti
Copy link
Contributor Author

jviotti commented Aug 3, 2017

I think we're the pioneers :P

});

app.controller('StateController', function($rootScope, $scope) {
app.controller('StateController', function ($rootScope, $scope) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity (this may well be another one of my stupid questions) how come this is still using the function keyword rather than fat-arrow notation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Angular relies on being able to set custom contexts with .call() and .apply(), which fat arrow functions don't support. Basically, Angular.js 2 is not ES6 ready.

super(props);

this.state = {
shouldShow: true
};

const url = new URL(props.src);
const url = new window.URL(props.src);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Shou is this correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup! URL lives on window, and this makes it explicit, so a good change.

@jhermsmeier
Copy link
Contributor

Out of curiosity, have any / many other resin.io repos switched to this new style?

@lurch yup https://github.com/resin-io/resin-preload

@lurch
Copy link
Contributor

lurch commented Aug 5, 2017

@jhermsmeier Yup, saw that ;-)

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.

5 participants