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

Update: add autofixing to test-case-property-ordering. (fixes #31) #32

Merged
merged 5 commits into from
Jul 31, 2017

Conversation

aladdin-add
Copy link
Contributor

@aladdin-add aladdin-add commented Jul 29, 2017

enable it on eslint codebase: eslint/eslint#9040

my concern is we didn't enable object-curly-newline, this may fixing some code like this:

{ code: "var [ x, y ] = z",
options: ["always"],
parserOptions: { ecmaVersion: 6 } },

looks so, hmm... strange!

@not-an-aardvark
Copy link
Contributor

my concern is we didn't enable object-curly-newline, this may fixing some code like this:

{ code: "var [ x, y ] = z",
options: ["always"],
parserOptions: { ecmaVersion: 6 } },

Hmm, this doesn't seem ideal. Why not keep the old spacing, and just swap the properties around? That way, comments will also be preserved.

@aladdin-add aladdin-add force-pushed the issue31 branch 2 times, most recently from 021d9bc to e1a202b Compare July 30, 2017 21:17
@aladdin-add
Copy link
Contributor Author

ping @not-an-aardvark

let trailing = source.slice(propertyStart, propertyEnd);

// for last property, should check & add trailling commas.
if (j === properties.length - 1 && sourceCode.getTokenAfter(last).value !== ',') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a special case for trailing commas, would it work to just set finalEnd to the trailing comma if it exists?

const finalEnd = sourceCode.getTokenAfter(last);

const insertedIndex = orderMsg.indexOf(keyNames[j]);
const propertyCode = sourceCode.getText(properties[j]);
const propertyStart = properties[j].range[1];
const propertyEnd = j < properties.length - 1 ? properties[j + 1].range[0] : finalEnd;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these variable names are confusing because propertyStart and propertyEnd describe the boundaries of the whitespace after the property, not the property itself. Maybe whitespaceStart and whitespaceEnd would be better.

[start, finalEnd],
result.join('')
);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to use the new feature in ESLint 4.1.0 where you can return an array of autofixes?

That way, this autofix would be very simple, because it wouldn't be necessary to deal with whitespace or trailing commas:

fix(fixer) {
  return orderMsg.map((key, index) => {
    const propertyToInsert = test.properties[keyNames.indexOf(key)];
    return fixer.replaceText(test.properties[index], sourceCode.getText(propertyToInsert));
  });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome!!!

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@aladdin-add aladdin-add merged commit 23f9010 into eslint-community:master Jul 31, 2017
@aladdin-add aladdin-add deleted the issue31 branch July 31, 2017 00:21
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.

None yet

2 participants