Skip to content

Proposal: jsx-child-element-spacing proposal #1519

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

Merged
merged 8 commits into from
Dec 1, 2017
Merged

Proposal: jsx-child-element-spacing proposal #1519

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a6e39d0
Failing tests for jsx-child-element-spacing proposal
pfhayes Nov 7, 2017
0922c56
Add some more test cases
pfhayes Nov 8, 2017
cc34480
Temp commit
pfhayes Nov 15, 2017
9de7c73
Cleanup
pfhayes Nov 15, 2017
5b2e1d4
Remove some cases
pfhayes Nov 15, 2017
bda68ce
Merge remote-tracking branch 'upstream/master' into spaces
pfhayes Nov 16, 2017
c8ee193
Avoid incldues
pfhayes Nov 16, 2017
7def79f
Review comments
pfhayes Nov 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions lib/rules/jsx-child-element-spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
'use strict';

// TODO(pfhayes): Just a stub for now to get tests working
module.exports = {
meta: {
docs: {},
schema: [
{
type: 'object',
properties: {},
default: {},
additionalProperties: false
}
]
},
create: function (/* context */) {
return {};
}
};
73 changes: 73 additions & 0 deletions tests/lib/rules/jsx-child-element-spacing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';

const rule = require('../../../lib/rules/jsx-child-element-spacing');
const RuleTester = require('eslint').RuleTester;
const parserOptions = {
sourceType: 'module',
ecmaFeatures: {
jsx: true
}
};

const ERROR_MESSAGE = [{message: 'Ambiguous spacing between child elements.'}];

const ruleTester = new RuleTester({parserOptions});
ruleTester.run('jsx-child-element-spacing', rule, {
valid: [{
code: `
<App>
foo
bar
</App>
`
}, {
code: `
<App>
<a>foo</a>
<a>bar</a>
</App>
`
}, {
code: `
<App>
foo<a>bar</a>baz
</App>
`
}, {
code: `
<App>
foo
{' '}
<a>bar</a>
{' '}
baz
</App>
`
}, {
code: `
<App>
foo{/*
*/}<a>bar</a>{/*
*/}baz
</App>
`
}],

invalid: [{
code: `
<App>
foo
<a>bar</a>
Copy link
Member

Choose a reason for hiding this comment

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

what would the output be if you wanted this autofixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think as you pointed out in the original issue, the intent is ambiguous so autofixing may not be feasible.

If we were to autofix, replacing with the comment form would maintain the semantic without too much disruption (though would perhaps be less aesthetically pleasing)

I would advocate for no automatic action since, in my experience, the developer most often intended to add a space, though I could be convinced otherwise

</App>
`,
errors: ERROR_MESSAGE
}, {
code: `
<App>
<a>bar</a>
baz
</App>
`,
errors: ERROR_MESSAGE
}]
});