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

Linter v5 Compatibility #3478

Merged

Conversation

misteroneill
Copy link
Member

Description

This makes the source code pass v5 of videojs-standard.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@misteroneill misteroneill added the minor This PR can be added to a minor release. It should not be added to a patch release. label Jul 29, 2016
@misteroneill misteroneill added this to the Linting milestone Jul 29, 2016
@misteroneill misteroneill added needs: LGTM Needs one or more additional approvals and removed minor This PR can be added to a minor release. It should not be added to a patch release. labels Aug 2, 2016
@@ -3015,10 +3015,10 @@ Player.prototype.handleVolumeChange_; // eslint-disable-line
*
* @event error
*/
Player.prototype.handleError_ = Player.prototype.handleError_;
Player.prototype.handleError_; // eslint-disable-line
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this work and is it needed? seems like it is never used from a quick code search

Copy link
Member

@gkatsev gkatsev Aug 2, 2016

Choose a reason for hiding this comment

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

it's used for the jsdoc @event comment.

@brandonocasey
Copy link
Contributor

LGTM other than my small comments

@@ -1452,7 +1452,7 @@ class Component {
subObj.extend = Component.extend;

// Extend subObj's prototype with functions and other properties from props
for (let name in props) {
for (const name in props) {
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this shouldn't be a let?

@gkatsev
Copy link
Member

gkatsev commented Aug 2, 2016

I guess const in a for..in is fine? If so,
LGTM.

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels Aug 2, 2016
@misteroneill
Copy link
Member Author

Yeah, ESLint complains about it. Which makes some sense, I suppose.

@misteroneill misteroneill merged this pull request into videojs:vjsstandard-core Aug 3, 2016
@misteroneill misteroneill deleted the vjsstandard-src-v5 branch August 3, 2016 19:29
misteroneill added a commit to misteroneill/video.js that referenced this pull request Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants