Fix: eslint #385

omgaz · 2016-10-21T06:19:46Z

ESLint has been broken for a few months now.

Problem has existed since we migrated from .js to ,jsx for our jsx files as per airbnb eslint rules.

Update pre-commit hooks to only check staged files
Updated messages when running pre-commit
Added magnifying glass 🔍 when checking things out
Added broken hearts 💔 when something goes wrong
Renamed lint script to eslint
Added new lint script that lints everything (not just eslint)
Mapped posttesttolint`
Fix broken lint

omgaz · 2016-10-21T06:22:27Z

README.md

@@ -56,7 +56,7 @@ npm run dist

 # Lint all files in src (also automatically run after tests)

-npm run lint


This should still be lint

vinteo

LGTM

OndrejCholeva · 2016-10-21T06:35:20Z

src/components/adslotUi/PagedGridComponent.jsx

@@ -50,7 +50,7 @@ class PagedGridComponent extends React.Component {
        {totalPages > 1 ?
          <div className="pagedgrid-component-pagination">
            <span className="pagedgrid-component-pagination-info">
-              {(activePage - 1) * perPage + 1}–{Math.min(activePage * perPage, items.length)} of {items.length}
+              {((activePage - 1) * perPage) + 1}–{Math.min(activePage * perPage, items.length)} of {items.length}


is this necessary?

The lint rules comlain about mixed * and +. I'd have assumed bodmas would take precedence.

ESLint defines the benefit as:

Enclosing complex expressions by parentheses clarifies the developer’s intention, which makes the code more readable. This rule warns when different operators are used consecutively without parentheses in an expression.

Seriously ESLint? I thought that this is only for Facebook kids, not devs...

maybe can use the rule with groups: http://eslint.org/docs/rules/no-mixed-operators#groups which makes much more sense (only forcing parens around logical and bitwise operators).

Agreed, sounds good :)

OndrejCholeva · 2016-10-21T06:38:56Z

src/components/adslotUi/TreePickerNodeComponent.jsx

+          .map('label')
+          .join(', ')
+      }
+    </span> : null;


should be:

</span> : null;

but I don't think putting semicolons on own line improves anything...

Null is kinda the exception I think; @patrick-ivers?

codecov-io · 2016-10-21T06:40:53Z

Current coverage is 100% (diff: 100%)

Merging #385 into master will not change coverage

@@           master   #385   diff @@
====================================
  Files          24     24          
  Lines         401    402     +1   
  Methods        62     64     +2   
  Messages        0      0          
  Branches       73     72     -1   
====================================
+ Hits          401    402     +1   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 1a800a4...a2a307d

OndrejCholeva · 2016-10-21T06:41:56Z

scripts/git-hooks/pre-commit


-echo "…done."
+echo "🐨  Checking commit message.";


does it make sense to show koalas with informative messages? Checking commit message. is neither good nor bad.

OndrejCholeva · 2016-10-24T00:07:14Z

src/components/adslotUi/TreePickerNodeComponent.jsx

  const pathElement = !(_.isEmpty(node.path) && _.isEmpty(node.ancestors)) ?
    <span className={`${baseClass}-path`}>
-      { !_.isEmpty(node.path) ? printPathText(node) : printAncestorText(node) }


This was definitely more descriptive/readable. What was the reason for the change/what's wrong with arrow functions http://eslint.org/docs/rules/func-style#allowarrowfunctions?

Good spot, I meant to move these further up :)

The issue here was the added complexity of defining functions inside the render method and re-using the component-scope node variable.

omgaz · 2016-10-24T01:39:56Z

Ran direct-web e2e: https://ci.adslot.com/view/Direct/job/ui-test-direct/5328/ all passing
Tested in IE, all good :)

OndrejCholeva · 2016-10-24T01:57:36Z

test/components/adslotUi/ListPickerComponentTest.jsx

-    expect(itemInfoPropertyElements.at(2).find(GridCell).last().children().text()).to.equal('21');
-    expect(itemInfoPropertyElements.at(2).find(GridCell).last().prop('dts')).to.equal('age');
+    expect(itemInfoPropertyElements.at(1).find(GridCell)
+      .first()


are we forcing chains in tests as well or is this due to length constraint?

Length here, I believe; although in this instance the accessors are being repeated. I'll clean it up; but what are your thoughts on the chaining?

I think multi-line chains are easier to read/simplify especially when there is any nesting. However with exception of chains like selecting().one().particular().element.property (i.e. no mutating or complex set of dynamic rules to get the result, but just a simple way to navigate a relatively static object).

Also in this case I don't like the partial wrapping.

- Update pre-commit hooks to only check staged files - Updated messages when running pre-commit - Added magnifying glass 🔍 when checking things out - Added broken hearts 💔 when something goes wrong - Renamed `lint` script to `eslint` - Added new `lint` script that lints everything (not just eslint) - Mapped `post`test` to `lint` - Update eslint mixed-operators rules to only enforce for groups of bitwise and logical operators

vinteo · 2016-10-24T03:30:13Z

Merging...

omgaz changed the title ~~Fix eslint~~ Fix: eslint Oct 21, 2016

omgaz commented Oct 21, 2016

View reviewed changes

vinteo approved these changes Oct 21, 2016

View reviewed changes

omgaz force-pushed the fix-eslint branch from a312bd8 to 6526857 Compare October 21, 2016 06:35

OndrejCholeva reviewed Oct 21, 2016

View reviewed changes

omgaz force-pushed the fix-eslint branch 2 times, most recently from cf1f173 to 74df50e Compare October 24, 2016 00:02

OndrejCholeva reviewed Oct 24, 2016

View reviewed changes

omgaz force-pushed the fix-eslint branch from 74df50e to cbd0040 Compare October 24, 2016 01:36

OndrejCholeva reviewed Oct 24, 2016

View reviewed changes

omgaz force-pushed the fix-eslint branch from cbd0040 to a2a307d Compare October 24, 2016 03:20

vinteo merged commit 09d9e7e into master Oct 24, 2016

vinteo deleted the fix-eslint branch October 24, 2016 03:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: eslint #385

Fix: eslint #385

omgaz commented Oct 21, 2016 •

edited

Loading

omgaz Oct 21, 2016

vinteo left a comment

OndrejCholeva Oct 21, 2016

omgaz Oct 21, 2016

OndrejCholeva Oct 21, 2016 •

edited

Loading

OndrejCholeva Oct 21, 2016

omgaz Oct 23, 2016

OndrejCholeva Oct 21, 2016

omgaz Oct 21, 2016

codecov-io commented Oct 21, 2016 •

edited

Loading

OndrejCholeva Oct 21, 2016

OndrejCholeva Oct 24, 2016

omgaz Oct 24, 2016

omgaz Oct 24, 2016

omgaz commented Oct 24, 2016 •

edited

Loading

OndrejCholeva Oct 24, 2016

omgaz Oct 24, 2016

OndrejCholeva Oct 24, 2016

vinteo commented Oct 24, 2016

		@@ -56,7 +56,7 @@ npm run dist

		# Lint all files in src (also automatically run after tests)

		npm run lint

Fix: eslint #385

Fix: eslint #385

Conversation

omgaz commented Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

vinteo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejCholeva Oct 21, 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

codecov-io commented Oct 21, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

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

omgaz commented Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vinteo commented Oct 24, 2016

omgaz commented Oct 21, 2016 •

edited

Loading

OndrejCholeva Oct 21, 2016 •

edited

Loading

codecov-io commented Oct 21, 2016 •

edited

Loading

omgaz commented Oct 24, 2016 •

edited

Loading