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.

Sign up for GitHub

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

Jump to bottom

jsx-wrap-multilines now catches single missing newlines #1984

Merged
merged 2 commits into from Oct 31, 2018
Merged

jsx-wrap-multilines now catches single missing newlines #1984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51f9c6a
jsx-wrap-multilines now catches single missing newlines (#1965)
MrHen Sep 16, 2018
82e4f41
Use explicit fixes for tests
MrHen Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 39 additions & 6 deletions lib/rules/jsx-wrap-multilines.js
View file
Open in desktop
Expand Up @@ -94,13 +94,32 @@ module.exports = {
nextToken.value === ')' && nextToken.range[0] >= node.range[1];
}

function needsNewLines(node) {
function needsOpeningNewLine(node) {
const previousToken = sourceCode.getTokenBefore(node);

if (!isParenthesised(node)) {
return false;
}

if (previousToken.loc.end.line === node.loc.start.line) {
return true;
}

return false;
}

function needsClosingNewLine(node) {
const nextToken = sourceCode.getTokenAfter(node);

return isParenthesised(node) &&
previousToken.loc.end.line === node.loc.start.line &&
node.loc.end.line === nextToken.loc.end.line;
if (!isParenthesised(node)) {
return false;
}

if (node.loc.end.line === nextToken.loc.end.line) {
return true;
}

return false;
}

function isMultilines(node) {
Expand Down Expand Up @@ -150,8 +169,22 @@ module.exports = {
} else {
report(node, MISSING_PARENS, fixer => fixer.replaceText(node, `(\n${sourceCode.getText(node)}\n)`));
}
} else if (needsNewLines(node)) {
report(node, PARENS_NEW_LINES, fixer => fixer.replaceText(node, `\n${sourceCode.getText(node)}\n`));
} else {
const needsOpening = needsOpeningNewLine(node);
const needsClosing = needsClosingNewLine(node);
if (needsOpening || needsClosing) {
report(node, PARENS_NEW_LINES, fixer => {
const text = sourceCode.getText(node);
let fixed = text;
if (needsOpening) {
fixed = `\n${fixed}`;
}
if (needsClosing) {
fixed = `${fixed}\n`;
}
return fixer.replaceText(node, fixed);
});
}
}
}
}
Expand Down
60 changes: 60 additions & 0 deletions tests/lib/rules/jsx-wrap-multilines.js
View file
Open in desktop
Expand Up @@ -86,6 +86,56 @@ const RETURN_PAREN_NEW_LINE = `
});
`;

const RETURN_PAREN_NEW_LINE_OPENING = `
var Hello = createReactClass({
render: function() {
return (

Copy link
Member

Choose a reason for hiding this comment

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

These extra newlines here seem odd - why is this allowed to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my earlier comment:
#1984 (comment)

My first attempts at fixing this caused extraneous newlines to disappear which seemed like an undesired side-effect of this particular rule. The extra newline isn't being added by the fix -- its in the original test condition. The test is making sure the extra newline is not being stripped.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense (although I’d want it stripped, this probably isn’t the rule for it)

<div>
<p>Hello {this.props.name}</p>
</div>);
}
});
`;

const RETURN_PAREN_NEW_LINE_OPENING_FIXED = `
var Hello = createReactClass({
render: function() {
return (

<div>
<p>Hello {this.props.name}</p>
</div>
);
}
});
`;

const RETURN_PAREN_NEW_LINE_CLOSING = `
var Hello = createReactClass({
render: function() {
return (<div>
<p>Hello {this.props.name}</p>
</div>

);
}
});
`;

const RETURN_PAREN_NEW_LINE_CLOSING_FIXED = `
var Hello = createReactClass({
render: function() {
return (
<div>
<p>Hello {this.props.name}</p>
</div>

);
}
});
`;

const RETURN_PAREN_NEW_LINE_FRAGMENT = `
var Hello = createReactClass({
render: function() {
Expand Down Expand Up @@ -912,6 +962,16 @@ ruleTester.run('jsx-wrap-multilines', rule, {
output: addNewLineSymbols(RETURN_PAREN),
options: [{return: 'parens-new-line'}],
errors: [{message: PARENS_NEW_LINES}]
}, {
code: RETURN_PAREN_NEW_LINE_OPENING,
output: RETURN_PAREN_NEW_LINE_OPENING_FIXED,
options: [{return: 'parens-new-line'}],
errors: [{message: PARENS_NEW_LINES}]
}, {
code: RETURN_PAREN_NEW_LINE_CLOSING,
output: RETURN_PAREN_NEW_LINE_CLOSING_FIXED,
options: [{return: 'parens-new-line'}],
errors: [{message: PARENS_NEW_LINES}]
}, {
code: RETURN_PAREN_FRAGMENT,
parser: 'babel-eslint',
Expand Down