Skip to content

Conversation

kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Aug 1, 2016

What issue does this pull request address?
#6407

What changes did you make? (Give an overview)
Enabled object-shorthand rule and ran auto-fix on codebase.

Command used to make changes:
bin/eslint.js --no-eslintrc --rule 'object-shorthand: 2' --env 'es6' --fix lib bin conf packages tests Makefile.js

Is there anything you'd like reviewers to focus on?
Would love a few more pairs of eyes to scan all the changes to make sure they look good!

Pretty straightforward, aside from two things:

  • Had to move a /* istanbul ignore next */ comment to work for the shorthand notation (see this issue in the Istanbul repo for more info)
  • There's a bug in Node v4.x.x when using get as a shorthand property name. We actually saw a bug report for this very situation and decided against fixing it, so my solution was to just rename the function. Will comment the specific location in the code.

initializeConfig: /* istanbul ignore next */ function(callback) {
getConfigForStyleGuide,
processAnswers,
/* istanbul ignore next */initializeConfig(callback) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Istanbul comment fix

@@ -65,7 +65,7 @@ function importPlugin(pluginRules, pluginName) {
* @param {string} ruleId Rule id (file name).
* @returns {Function} Rule handler.
*/
function get(ruleId) {
function getHandler(ruleId) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Function renamed to avoid Node v4.x.x bug that treats shorthand properties named get as syntax errors.

@eslintbot
Copy link

LGTM

@platinumazure
Copy link
Member

platinumazure commented Aug 1, 2016

LGTM, thanks for contributing! (Got a mocha timeout in the CLIEngine tests when running locally, but I can't imagine that would have anything to do with your changes.)

@nzakas
Copy link
Member

nzakas commented Aug 1, 2016

@kaicataldo looks like a rebase is needed.

@kaicataldo kaicataldo force-pushed the objectshorthand branch 2 times, most recently from d63eb5a to 78e0a19 Compare August 2, 2016 03:51
@eslintbot
Copy link

LGTM

@kaicataldo
Copy link
Member Author

Rebased

@gyandeeps
Copy link
Member

@kaicataldo Sorry but one more time plz. 😄

@nzakas
Copy link
Member

nzakas commented Aug 2, 2016

Maybe we should try splitting this up into batches?

@kaicataldo
Copy link
Member Author

@nzakas Probably a good idea - I'll do that tonight.

@kaicataldo
Copy link
Member Author

I'm not actually going to be able to get to this tonight, unfortunately. Will get to it ASAP.

@kaicataldo kaicataldo closed this Aug 12, 2016
@kaicataldo kaicataldo deleted the objectshorthand branch August 12, 2016 19:29
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants