Skip to content

eslint config update & lintfixes#95

Merged
arizzitano merged 1 commit intomasterfrom
ari/eslint-es5-3.0.0-lintfixes
Apr 21, 2017
Merged

eslint config update & lintfixes#95
arizzitano merged 1 commit intomasterfrom
ari/eslint-es5-3.0.0-lintfixes

Conversation

@arizzitano
Copy link
Contributor

@arizzitano arizzitano commented Apr 20, 2017

Description

  • Upgrade eslint
  • Upgrade eslint-config-edx and eslint-config-edx-es5
  • Fix resulting eslint errors

Post-review

  • Squash

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.848% when pulling 0260486 on ari/eslint-es5-3.0.0-lintfixes into 173b1be on master.

"edx-pattern-library": "~0.12.5",
"eslint": "~2.13.1",
"eslint-config-edx": "~1.2.1",
"eslint": "^3.16.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK we don't need an explicit ESLint dependency anymore now that we're on NPM 3 (although maybe the Yarn thing you found has implications for this?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we do need it since Yarn won't link binaries for transitive deps and recommends either an explicit dependency or a postinstall creating the link manually. The explicit dependency seemed like a cleaner option in this case.

requests.currentIndex = 0;
requests.restore = function() {
if (xhr && xhr.hasOwnProperty('restore')) {
if (xhr && Object.prototype.hasOwnProperty.call(xhr, 'restore')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a little yucky - does && 'restore' in xhr work here?


expect(requests instanceof Array).toBeTruthy();
expect(requests.hasOwnProperty('currentIndex')).toBeTruthy();
expect(Object.prototype.hasOwnProperty.call(requests, 'currentIndex')).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to avoid these Object.prototype.x.call calls if we can, see above

Copy link
Contributor Author

@arizzitano arizzitano Apr 20, 2017

Choose a reason for hiding this comment

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

Agreed this is gross. 'prop' in object works here afaik and is much prettier 👍

@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.848% when pulling 8ef0754 on ari/eslint-es5-3.0.0-lintfixes into 173b1be on master.

Copy link
Contributor

@andy-armstrong andy-armstrong left a comment

Choose a reason for hiding this comment

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

👍

eslint autofix

manual eslint fixes

less ugly hasOwnProperty alternative
@arizzitano arizzitano force-pushed the ari/eslint-es5-3.0.0-lintfixes branch from 8ef0754 to 3f8351d Compare April 21, 2017 14:04
@coveralls
Copy link

Coverage Status

Coverage remained the same at 98.848% when pulling 3f8351d on ari/eslint-es5-3.0.0-lintfixes into 173b1be on master.

@arizzitano arizzitano merged commit 788ae12 into master Apr 21, 2017
@arizzitano arizzitano deleted the ari/eslint-es5-3.0.0-lintfixes branch April 21, 2017 14:12
@arizzitano arizzitano mentioned this pull request Apr 21, 2017
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