[WIP] Use XO instead of Standard #723

matheuss · 2016-09-20T04:27:25Z

TODO:

Fix React errors

There was a discussion in zeit.chat about changing the code style of HyperTerm's code base – and later on @zeit's code base (via @leo). We compared XO, Standard and Airbnb's JavaScript Style Guide.

Since I use XO in my projects and I'm very happy with the rules it enforces, I'm sending this PR so we can discuss changing HyperTerm's code style to XO's 😄

There was a lot of changes – many of them had to be done manually (xo --fix helped in the simpler ones), so I created a summary:

Summary of changes

const { something } = require('module'); => const {something} = require('module');

fn({ key: 'value' }); => fn({key: 'value'});

function name () {...} => function name() {...}

(arg) => {...} => arg => {...}

<Tag prop='value'> => <Tag prop="value">

return <Tag>; => return (<Tag>);

something("'"); => something('\'');

let a, b, c; => one let per line

if (value !== 0) {
  // value is not zero
} else {
  // value is zero
}

was changed to

if (value === 0) {
  // value is zero
} else {
  // value is not zero
}

something
? path1
: path2;

was changed to

something ?
path1 :
path2;

if ('object' !== typeof value) => if (typeof value !== 'object')

if (something) {
  return true;
} else {
  return false;
}

was changed to

if (something) {
  return true;
}
return false;

if (something) return true;
was changed to

if (something) {
  return true;
}

obj. hasOwnProperty(prop); => {}. hasOwnProperty.call(obj, prop);

==, !== => ===, !==

Another changes

The store variable in Redux middlewares was removed when it's not used – 8a8fdac#diff-6c6a3219ca05ccbe3ce89f7544a8f40aR8

Some rules relating to switch-case were disabled because the code was kinda complex and I didn't want to change it – 8a8fdac#diff-7da6d687f5a3beed5725bb8324e4b1adR82

The babel/new-cap rule was disabled due to things like this: 8a8fdac#diff-8303185c505c28257453bad1aadc768bR22

For some reason (probably related to this binding) the (only) test we have couldn't be changed from standard functions to arrow functions – 8a8fdac#diff-f7cc7aca73879ae481a889ab216ba926R1

React rules

There's currently 151 errors related to React code styling. Since I have 0 expertise with React, I didn't change anything related to it. Any help is more than welcome 😄

Conclusion

Since these changes affect every contributor, we need to discuss them carefully. I'm 100% open to any help, input and feedback about them.

A lot of the changes can affect how the code works, so any help with testing it is very welcome.

…inter

leo

Something is wrong with the tabs. A key isn't being assigned properly. After this works, we can merge it!

leo · 2016-09-21T14:26:36Z

The tab thing is actually broken on master. Not the result of our changes!

leo · 2016-09-21T14:33:20Z

cc @sindresorhus

Fixes a bug introduced by #723

ekmartin · 2016-09-21T19:00:59Z

lib/actions/ui.js

        const uid = sessions.activeUid;
        const sessionUids = keys(sessions.sessions);
        if (uid === sessionUids[i]) {
          console.log('ignoring same uid');
-        } else if (null != sessionUids[i]) {


null != sessionUids[i] checks for undefined as well @matheuss, now you're explicitly checking for the value null

That's true @ekmartin 😰 What do you think we should do?

Set eqeqeq to ["error", "always", {"null": "ignore"}] and revert the == null and != null changes maybe?

http://eslint.org/docs/rules/eqeqeq#allow-null

I don't know what makes sense in this context, but in general, checking against both null and undefined is an anti-pattern these days. Native things like "default parameters" only work on undefined, not null, so better to get used to null meaning an empty value, and undefined meaning no value.

Generally I agree, however I'm a bit unsure when it comes to config values. If it never makes sense for a specific config value to be null it might be better to prevent plugins and similar from setting it as such.

Then I would rather throw a TypeError and be explicit about it.

But if you really want both undefined and null, I would strongly recommend just being verbose and explicit about it: sessionUid[i] !== undefined && sessionUid[i] !== null. Not everyone knows that sessionUid[i] != null coerces to the same. XO is about enforcing explicit and readable code.

Sounds reasonable :)

ekmartin · 2016-09-21T19:02:59Z

lib/hterm.js

-  var text = this.getSelectionText();
-  if (text != null && text !== '') {
+  const text = this.getSelectionText();
+  if (text !== null && text !== '') {


same here, text as undefined would get through text !== null but not text != null. Maybe if (text) would suffice

ekmartin · 2016-09-21T19:03:20Z

lib/reducers/ui.js

@@ -91,71 +91,69 @@ const reducer = (state = initial, action) => {
            ret.fontSizeOverride = null;
          }

-          if (null != config.fontSize) {


sindresorhus · 2016-09-22T03:55:26Z

Awesome! Let me know if you have any concerns or questions. I'm happy to discuss any of the rule choices. There's a good reasoning behind every single rule.

sindresorhus · 2016-09-22T04:14:58Z

package.json

+      "react/no-string-refs": 0,
+      "react/jsx-key": 0,
+      "no-nested-ternary": 0,
+      "react/jsx-no-bind": 0
    },


@matheuss @leo I would like to help you get this down closer to zero. Especially the React rules are open for discussion. This would also be value feedback for me.

#723 (comment)

sindresorhus · 2016-09-22T04:15:42Z

package.json

+      "react/no-danger": 0,
+      "react/no-string-refs": 0,
+      "react/jsx-key": 0,
+      "no-nested-ternary": 0,


It's only a warning, but just removed this as I don't like how it works either: xojs/eslint-config-xo@d3944f6 (Planning a new release in a few weeks)

sindresorhus · 2016-09-22T04:16:05Z

package.json

+      "babel/new-cap": 0,
+      "quote-props": 0,
+      "import/no-extraneous-dependencies": 0,
+      "no-warning-comments": 0,


Why? It's useful to be reminded about TODO comments.

#723 (comment)

sindresorhus · 2016-09-22T04:16:57Z

package.json

+      "react/prop-types": 0,
+      "babel/new-cap": 0,
+      "quote-props": 0,
+      "import/no-extraneous-dependencies": 0,


I assume this is because of complaining about the electron import. This will be fixed with eslint-plugin-import@2 where they will allow XO to specify the builtin.

#723 (comment)

sindresorhus · 2016-09-22T04:18:54Z

package.json

+      "quote-props": 0,
+      "import/no-extraneous-dependencies": 0,
+      "no-warning-comments": 0,
+      "complexity": 0,


Why? Complexity is usually a good metric of too many code paths. Was it too spammy? Should I increase the maximum (currently 20)?

#723 (comment)

sindresorhus · 2016-09-22T05:10:45Z

test/app/plugins.js

@@ -1,3 +1,4 @@
+/* eslint-disable prefer-arrow-callback */


Why not just use arrow functions?

sindresorhus · 2016-09-22T05:11:05Z

webpack.config.js

@@ -1,6 +1,7 @@
+const path = require('path');
+


No need for an empty line here.

sindresorhus · 2016-09-22T05:18:14Z

lib/utils/file.js

-    (fileStat['mode'] & parseInt('0100', 8))
+    (fileStat.mode & parseInt('0001', 8)) ||
+    (fileStat.mode & parseInt('0010', 8)) ||
+    (fileStat.mode & parseInt('0100', 8))


No rule for this yet, but these should use octal literals instead of parseInt.

http://www.2ality.com/2015/04/numbers-math-es6.html

sindresorhus · 2016-09-22T05:19:53Z

lib/hterm.js

@@ -188,7 +189,7 @@ lib.colors.hexToRGB = function (arg) {
  }

  if (arg instanceof Array) {
-    for (var i = 0; i < arg.length; i++) {
+    for (let i = 0; i < arg.length; i++) {


Here it would be better to use a for..of loop.

No rule for it yet, but consider me a linter :p

sindresorhus · 2016-09-22T05:20:16Z

lib/hterm.js

@@ -188,7 +189,7 @@ lib.colors.hexToRGB = function (arg) {
  }

  if (arg instanceof Array) {


Use Array.isArray(). instanceof won't work across realms (frames/iframes/vm).

leo · 2016-09-22T05:30:44Z

@sindresorhus Just for clarity: Yes, disabling these rules is definitely a bad practise and I completely agree with you. Which is why we're going to break them down to zero. The only reason why I haven't did that before merging the PR was because it's a lot of work... 😊

As you can see in the issue description, I already put some effort into making the code comply with the rules, but these ones are left.

I would like to help you get this down closer to zero...

We're thankful for every helpful hand that we can get (especially for yours)!

sindresorhus · 2016-09-22T05:44:49Z

Ah, makes sense. I should have looked at the issue tracker before commenting.

matheuss added 5 commits September 19, 2016 23:30

Bump eslint-plugin-react

21d52d1

Add eslint-config-xo-react

da7aafd

Add XO

2dde977

Remove eslint-related dependencies, add XO config and use XO as the l…

c7c5780

…inter

Code style: Standard => XO ✨

8a8fdac

leo added 3 commits September 21, 2016 15:00

Merge branch 'master' into xo

3c54e8a

Use xo property to ignore files

ea72a99

Fix remaining errors

f7517f2

leo suggested changes Sep 21, 2016

View reviewed changes

leo added help wanted Contributions wanted towards the issue and removed 💚 Priority: Low Issue is "nice-to-have" or not important to the core app 💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ Status: Review Needed labels Sep 21, 2016

leo merged commit 1866104 into master Sep 21, 2016

leo deleted the xo branch September 21, 2016 14:27

matheuss mentioned this pull request Sep 21, 2016

Fix tab navigation using mouse clicks #732

Merged

matheuss pushed a commit that referenced this pull request Sep 21, 2016

Fix tab navigation using mouse clicks (#732)

ac69e0b

Fixes a bug introduced by #723

ekmartin reviewed Sep 21, 2016

View reviewed changes

sindresorhus reviewed Sep 22, 2016

View reviewed changes

ekmartin mentioned this pull request Sep 23, 2016

Rewrite code to make xo happy without any rules #730

Closed

12 tasks

sindresorhus mentioned this pull request Sep 24, 2016

Comply XO's "jsx-filename-extension" rule #739

Closed

ekmartin mentioned this pull request Sep 30, 2016

Replace explicit null check with isNil #760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use XO instead of Standard #723

[WIP] Use XO instead of Standard #723

matheuss commented Sep 20, 2016 •

edited by leo

Loading

leo left a comment

leo commented Sep 21, 2016

leo commented Sep 21, 2016

ekmartin Sep 21, 2016

matheuss Sep 21, 2016

ekmartin Sep 21, 2016

sindresorhus Sep 22, 2016 •

edited

Loading

ekmartin Sep 22, 2016

sindresorhus Sep 22, 2016

ekmartin Sep 22, 2016

ekmartin Sep 21, 2016

ekmartin Sep 21, 2016

sindresorhus commented Sep 22, 2016 •

edited

Loading

sindresorhus Sep 22, 2016 •

edited

Loading

leo Sep 22, 2016

sindresorhus Sep 22, 2016

leo Sep 22, 2016

sindresorhus Sep 22, 2016

leo Sep 22, 2016

sindresorhus Sep 22, 2016

leo Sep 22, 2016

sindresorhus Sep 22, 2016

leo Sep 22, 2016

sindresorhus Sep 22, 2016

sindresorhus Sep 22, 2016

sindresorhus Sep 22, 2016

sindresorhus Sep 22, 2016

sindresorhus Sep 22, 2016

leo commented Sep 22, 2016

sindresorhus commented Sep 22, 2016

		@@ -188,7 +189,7 @@ lib.colors.hexToRGB = function (arg) {
		}

		if (arg instanceof Array) {

[WIP] Use XO instead of Standard #723

[WIP] Use XO instead of Standard #723

Conversation

matheuss commented Sep 20, 2016 • edited by leo Loading

Summary of changes

Another changes

React rules

Conclusion

leo left a comment

Choose a reason for hiding this comment

leo commented Sep 21, 2016

leo commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Sep 22, 2016 • edited Loading

sindresorhus Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leo commented Sep 22, 2016

sindresorhus commented Sep 22, 2016

matheuss commented Sep 20, 2016 •

edited by leo

Loading

sindresorhus Sep 22, 2016 •

edited

Loading

sindresorhus commented Sep 22, 2016 •

edited

Loading

sindresorhus Sep 22, 2016 •

edited

Loading