Enable curly spacing check for children #1177

fatfisz · 2017-05-07T13:20:06Z

This is a feature discussed in #857.

TODO:

move all the bugfixes to separate branches
merge the master after the fixes are merged
add more tests for attributes and children
add more tests for config inheritance
restore the old config behavior alongside the new one
update the README

fatfisz · 2017-05-07T13:27:17Z

I think I'll add a few tests that have both attributes and children set to check that the defaults are being properly set.

ljharb · 2017-05-07T19:27:50Z

lib/rules/jsx-curly-spacing.js

@@ -30,34 +32,87 @@ module.exports = {
    fixable: 'code',

    schema: [{
-      enum: SPACING_VALUES


this seems like it's removing the old schema, which makes it a breaking change - the idea would be to allow all existing configuration possibilities to continue to work, but provide an alternative that people could migrate to.

Specifically, this also means that (separate from bugfixes) zero existing tests should be modified, and should all continue to pass.

Ok, will do.

fatfisz · 2017-05-07T20:45:43Z

I mistakenly removed the branch -.-

ljharb · 2017-05-09T17:52:16Z

Seems like with #1179 and #1180 merged, this needs a rebase?

fatfisz · 2017-05-09T18:12:43Z

Sure.

ljharb · 2017-05-09T20:10:19Z

@fatfisz please rebase on the command line and force push, so that there's no merge commits

fatfisz · 2017-05-09T21:16:36Z

Rebased.

Just curious, why do you want to avoid merge commits? It's less time-consuming to solve the conflicts once; also the history is being lost.

fatfisz · 2017-05-09T21:46:49Z

I've added back the old tests (avoiding duplication) and restored the old config behavior. Should I leave any comments about what is from where?

ljharb · 2017-05-09T23:50:28Z

Git logs aren't history, they're change logs - nobody ever needs to know how many times you merged master. Also, merge conflict resolutions in a rebase are preserved for future cherry pickers; conflicts resolved in a merge potentially force developers to re-resolve the same conflicts. It also generally takes precisely the same amount of time to solve a conflict in a merge as it does in a rebase.

ljharb

This is looking great; just a few questions about the tests.

ljharb · 2017-05-09T23:51:37Z

lib/rules/jsx-curly-spacing.js

@@ -32,7 +32,52 @@ module.exports = {
    fixable: 'code',

    schema: [{
-      enum: SPACING_VALUES
+      definitions: {


i'm not familiar with the "definitions" rule schema key - what version of eslint added this?

I've just read in the ESLint docs that it's using JSON schema - then went over to the JSON schema reference page (this one is very good) and saw that this technique there.

After a little investigation, it should be supported since the schema validation itself was added: eslint/eslint@fd8fbe7. According to the changelog, the version was 0.22.0, released on 2015-05-30.

The version of is-my-json-valid used then is 2.10.0, which already handled refs: https://github.com/mafintosh/is-my-json-valid/blob/21aa9b8916a2efb3172d17d2d3d1b517668834c7/test/json-schema-draft4/ref.json

Btw. "definitions" is just a convention; the ref can reference any path in the schema.

Wow, awesome. I had no idea.

ljharb · 2017-05-09T23:55:03Z

tests/lib/rules/jsx-curly-spacing.js

+    code: '<App>{ { bar: true, baz: true } }</App>;',
+    options: [{children: false}]
+  }, {
+    code: '<App foo={bar} />;',


It seems like some of the <App foo={bar} /> tests have been removed - for any code pattern, we need explicit tests for a) no options, b) all explicit options on the old (current) schema, c) all explicit options on the new schema, d) autofixes

I changed a few of them to include children too - they should be at the top. I did not remove any tests, just enhanced a few. The tests without the options should mostly be at the top of the list.

Should I split them to test attributes and children separately?

Yes please :-) when existing test cases are utterly untouched, it adds confidence that a change is not breaking.

Ok, now all tests that are currently on the master branch are also added here.

fatfisz · 2017-05-10T08:51:42Z

I was asking because in my experience it takes much longer to do a rebase, especially when there are many commits already. E.g. yesterday merge took < 5 mins, but the rebase took over half an hour. But the argument about cherry-picking speaks to me.

Just for the future reference - would it be ok to squash the commits next time while rebasing?

ljharb · 2017-05-10T16:08:55Z

Commits should ideally be atomic and thus each represent a conceptual change. Naive squashing is fine, but it can cause two changes to be combined where they should be separate.

ljharb

Seems good

fatfisz · 2017-05-19T16:36:08Z

Sorry, forgot about updating the docs. Will do that around tomorrow, I'm pumped out after work today.

fatfisz · 2017-05-21T22:46:09Z

Took a bit more time, but here it is.

ljharb

LGTM!

This should get a review from at least one other collab before merging.

fatfisz · 2017-06-05T21:45:46Z

Would it be possible for some other collaborators to review this?

fatfisz · 2017-06-11T17:08:49Z

It seems that resolving conflicts through GitHub results in a merge. Will rebase instead.

Use the "spaces" name.

Previously when "spacing" was set and "allowMultiline" was missing, it was set to `undefined` and not `true` (as it should).

This change does not make sense by itself, but it will with the added children config.

lencioni

Thanks for making this better!

lencioni · 2017-06-11T17:24:38Z

docs/rules/jsx-curly-spacing.md

@@ -135,6 +221,41 @@ When `"never"` is used and `objectLiterals` is `"always"`, the following pattern

 Please note that spacing of the object literal curly braces themselves is controlled by the built-in [`object-curly-spacing`](http://eslint.org/docs/rules/object-curly-spacing) rule.

+### Shorthand options


Do we see value in supporting these shorthand options indefinitely, or might we want to deprecate them to simplify in a major release? If we plan to deprecate them eventually, it might be worth noting that here so people can avoid using them now.

I'd say we might want to deprecate them (string options are horrible for extensibility), but there's not really any pressing need to.

ljharb · 2017-06-11T17:28:24Z

(The appveyor failure appears to be an npm install fluke, so i'm going to merge as-is)

fatfisz · 2017-06-11T17:31:55Z

Thanks a lot!

102 · 2017-06-13T04:31:09Z

@ljharb is there any ETA for this to be released?

ljharb · 2017-06-13T05:42:55Z

@102 #1251 (comment)

fatfisz force-pushed the curly-spacing-for-children branch from 30f36dd to 7ff31f5 Compare May 7, 2017 13:29

ljharb requested changes May 7, 2017

View reviewed changes

fatfisz closed this May 7, 2017

fatfisz deleted the curly-spacing-for-children branch May 7, 2017 20:43

fatfisz restored the curly-spacing-for-children branch May 7, 2017 20:45

fatfisz reopened this May 7, 2017

fatfisz force-pushed the curly-spacing-for-children branch from 0979686 to 998e356 Compare May 9, 2017 21:13

ljharb requested changes May 9, 2017

View reviewed changes

fatfisz force-pushed the curly-spacing-for-children branch from 98eb4f4 to 51466df Compare May 10, 2017 19:17

ljharb approved these changes May 10, 2017

View reviewed changes

ljharb added the enhancement label May 10, 2017

ljharb requested review from yannickcr, lencioni and EvHaus May 10, 2017 19:24

fatfisz force-pushed the curly-spacing-for-children branch from 2727e58 to 0c7a226 Compare May 21, 2017 22:43

ljharb approved these changes May 21, 2017

View reviewed changes

fatfisz added 9 commits June 11, 2017 19:09

Move the first option into the config object

0f0ab21

Use the "spaces" name.

Add a test case for a fixed bug

9a8f800

Previously when "spacing" was set and "allowMultiline" was missing, it was set to `undefined` and not `true` (as it should).

Allow attributes to have their own config

c9e9ded

This change does not make sense by itself, but it will with the added children config.

Change "spaces" to "when"

7b9d9ad

Fix the behavior of the boolean "attributes"

65135b8

Add the "children" property of the config

408e1b1

Add more mixed tests

5515130

Add back the support for the previous config

6903f18

Update the docs

d57c115

fatfisz force-pushed the curly-spacing-for-children branch from 912ec6b to d57c115 Compare June 11, 2017 17:10

lencioni approved these changes Jun 11, 2017

View reviewed changes

ljharb merged commit 32bcb48 into jsx-eslint:master Jun 11, 2017

ljharb mentioned this pull request Jun 11, 2017

react/jsx-curly-spacing not working #857

Closed

fatfisz mentioned this pull request Jun 11, 2017

Rule proposal: JavaScript Expression spacing #1007

Closed

renovate-bot mentioned this pull request Jun 15, 2017

Renovate eslint packages singapore/lint-condo#308

Merged

renovate bot mentioned this pull request Jul 7, 2017

Update dependency eslint-plugin-react to version 7.x algolia/react-element-to-jsx-string#102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable curly spacing check for children #1177

Enable curly spacing check for children #1177

fatfisz commented May 7, 2017 •

edited

fatfisz commented May 7, 2017

ljharb May 7, 2017

fatfisz May 7, 2017

fatfisz commented May 7, 2017

ljharb commented May 9, 2017

fatfisz commented May 9, 2017

ljharb commented May 9, 2017 •

edited

fatfisz commented May 9, 2017

fatfisz commented May 9, 2017

ljharb commented May 9, 2017

ljharb left a comment

ljharb May 9, 2017

fatfisz May 10, 2017

ljharb May 10, 2017

ljharb May 9, 2017

fatfisz May 10, 2017

ljharb May 10, 2017

fatfisz May 10, 2017

fatfisz commented May 10, 2017

ljharb commented May 10, 2017

ljharb left a comment

fatfisz commented May 19, 2017

fatfisz commented May 21, 2017

ljharb left a comment

fatfisz commented Jun 5, 2017

fatfisz commented Jun 11, 2017

lencioni left a comment

lencioni Jun 11, 2017

ljharb Jun 11, 2017

ljharb commented Jun 11, 2017

fatfisz commented Jun 11, 2017

102 commented Jun 13, 2017

ljharb commented Jun 13, 2017

		@@ -135,6 +221,41 @@ When `"never"` is used and `objectLiterals` is `"always"`, the following pattern

		Please note that spacing of the object literal curly braces themselves is controlled by the built-in [`object-curly-spacing`](http://eslint.org/docs/rules/object-curly-spacing) rule.

		### Shorthand options

Enable curly spacing check for children #1177

Enable curly spacing check for children #1177

Conversation

fatfisz commented May 7, 2017 • edited

TODO:

fatfisz commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatfisz commented May 7, 2017

ljharb commented May 9, 2017

fatfisz commented May 9, 2017

ljharb commented May 9, 2017 • edited

fatfisz commented May 9, 2017

fatfisz commented May 9, 2017

ljharb commented May 9, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fatfisz commented May 10, 2017

ljharb commented May 10, 2017

ljharb left a comment

Choose a reason for hiding this comment

fatfisz commented May 19, 2017

fatfisz commented May 21, 2017

ljharb left a comment

Choose a reason for hiding this comment

fatfisz commented Jun 5, 2017

fatfisz commented Jun 11, 2017

lencioni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jun 11, 2017

fatfisz commented Jun 11, 2017

102 commented Jun 13, 2017

ljharb commented Jun 13, 2017

fatfisz commented May 7, 2017 •

edited

ljharb commented May 9, 2017 •

edited