Update: add named imports and exports for object-curly-newline #9876

nicholaschuayunzhi · 2018-01-23T05:42:06Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Resolves #8501

What changes did you make? (Give an overview)
Added configuration for named imports and exports.

Is there anything you'd like reviewers to focus on?
Due to code reuse, the config allows for the consistent option which acts the same way in ObjectExpression and ObjectPattern. Would that be fine as it was not discussed in #8501.

Are the test cases comprehensive enough?

jsf-clabot · 2018-01-23T05:42:07Z

All committers have signed the CLA.

nicholaschuayunzhi · 2018-01-29T17:07:43Z

@platinumazure Would you be able to assist in getting this merged? Thank you :)

platinumazure · 2018-01-29T18:39:05Z

Hi @nicholaschuayunzhi, thanks for following up, this one slipped past me somehow. I'll review in the next day or two. Thanks!

platinumazure

Left some comments/questions. The only thing I would really want changed is the extraction of the needsLineBreaks check into its own function. Thanks for contributing and sorry again for letting this slip by!

platinumazure · 2018-02-02T18:13:06Z

docs/rules/object-curly-newline.md

@@ -19,19 +19,23 @@ Or an object option:
 * `"minProperties"` requires line breaks if the number of properties is at least the given integer. By default, an error will also be reported if an object contains linebreaks and has fewer properties than the given integer. However, the second behavior is disabled if the `consistent` option is set to `true`
 * `"consistent": true` requires that either both curly braces, or neither, directly enclose newlines. Note that enabling this option will also change the behavior of the `minProperties` option. (See `minProperties` above for more information)

-You can specify different options for object literals and destructuring assignments:
+You can specify different options for object literals, destructuring assignments, named imports and exports:


Nit: Could you please add "and" before "named imports and exports"? Thanks!

platinumazure · 2018-02-02T18:15:13Z

lib/rules/object-curly-newline.js

 */
 function normalizeOptions(options) {
-    if (options && (options.ObjectExpression || options.ObjectPattern)) {
+    if (options &&


Wondering if this could be replaced with something like lodash.isPlainObject(options)?

Considering that the options can take different forms, more checks must be made for more node specific options.

I've made changes in 75bea25 to make the code more readable.

Explanation:

"never" "always" { "multiline": true } { "minProperties": 2 } { "consistent": true } // want to identify this { "ObjectExpression": "always" } { "ObjectExpression": { "multiline": true } }

find options that are objects.

find the options that have a string or object as its value.

platinumazure · 2018-02-02T18:17:28Z

lib/rules/object-curly-newline.js

                closeBrace = sourceCode.getLastToken(node);
+            } else {
+                closeBrace = sourceCode.getTokenAfter(node.specifiers.slice(-1)[0], token => token.value === "}");


Could we use sourceCode.getLastToken(node, token => token.value === "}") in both this code path and the one for ObjectExpression/ObjectPattern?

platinumazure · 2018-02-02T18:18:16Z

lib/rules/object-curly-newline.js

-                )
-            );
+
+            let needsLinebreaks;


I think it would be good to extract this code into its own function to make things a little more readable.

platinumazure · 2018-02-02T18:19:26Z

tests/lib/rules/object-curly-newline.js

+                "} from 'module';"
+            ].join("\n"),
+            options: [{ ImportDeclaration: "always" }],
+            parserOptions: { sourceType: "module" }


You could add { parserOptions: { sourceType: "module" } } to the RuleTester constructor, in order to avoid having to specify it in each of your tests. I don't think there's a need for any test to specify { sourceType: "script" }, but let me know if I'm mistaken.

Thank you for the information. I'm not too sure about { sourceType: "script" }. To be honest, I'm learning from the other tests in the repo and I'm still unsure where to find more information about this.

platinumazure · 2018-02-02T18:22:13Z

docs/rules/object-curly-newline.md

+export { foo, bar } from 'foo-bar';
+export { foo as f, bar } from 'foo-bar';
+```
+
 ## Compatibility

 * **JSCS**: [requirePaddingNewLinesInObjects](http://jscs.info/rule/requirePaddingNewLinesInObjects) and [disallowPaddingNewLinesInObjects](http://jscs.info/rule/disallowPaddingNewLinesInObjects)


Is there a JSCS rule we are now compatible with for padding new lines for import/export specifiers?

There doesn't seem to be any JSCS rule regarding new lines and import/export specifiers. Only ones for spaces in the import braces.

requireSpacesInsideImportedObjectBraces
disallowSpacesInsideImportedObjectBraces

Thanks for checking!

nicholaschuayunzhi · 2018-02-03T05:25:19Z

Thank you for reviewing my pull request :) I have responded to some of the comments in the review and made changes as requested.

platinumazure

This looks great! I unfortunately was unclear on my earlier comment about the docs, so I apologize for requesting changes once again but once that is fixed, this should be ready to merge as a semver-minor change. Thanks so much for contributing!

platinumazure · 2018-02-03T05:27:10Z

docs/rules/object-curly-newline.md

@@ -19,19 +19,23 @@ Or an object option:
 * `"minProperties"` requires line breaks if the number of properties is at least the given integer. By default, an error will also be reported if an object contains linebreaks and has fewer properties than the given integer. However, the second behavior is disabled if the `consistent` option is set to `true`
 * `"consistent": true` requires that either both curly braces, or neither, directly enclose newlines. Note that enabling this option will also change the behavior of the `minProperties` option. (See `minProperties` above for more information)

-You can specify different options for object literals and destructuring assignments:
+You can specify different options for object literals and destructuring assignments, named imports and exports:


Sorry, I wasn't clear in my last comment. The removal of the original "and" was correct, but I wanted a new "and" before the named import/export.

This is what I was going for:

You can specify different options for object literals, destructuring assignments, and named imports and exports:

Thanks for your patience!

nicholaschuayunzhi · 2018-02-05T17:38:32Z

Hi i think I've got nothing left to change. Do I need to do anything else?

ilyavolodin · 2018-02-10T00:25:25Z

@platinumazure Could you check if all of your comments were addressed?

platinumazure

LGTM, thanks for contributing!

nicholaschuayunzhi · 2018-02-10T15:15:28Z

Thanks for your guidance!

nicholaschuayunzhi added 2 commits January 23, 2018 13:15

Update: add ImportDeclaration and ExportDeclaration

6463a8c

Chore: add tests for import/export declaration

728ad32

nicholaschuayunzhi added 2 commits January 23, 2018 13:43

Docs: update docs for object-curly-newline

e4017f7

Chore: update comment in object-curly-newline

dcef6de

nicholaschuayunzhi force-pushed the 8501-import-export-config branch from 3b51073 to dcef6de Compare January 23, 2018 05:44

Chore: fix JSDoc error

245b20e

aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Jan 23, 2018

platinumazure self-assigned this Jan 29, 2018

platinumazure requested changes Feb 2, 2018

View reviewed changes

nicholaschuayunzhi added 5 commits February 3, 2018 03:04

Chore: add sourceType module to RuleTester option

d6e275d

Update: combine logic for finding closing brace

9708cdb

Chore: extract function, needsLinebreaks

0020669

Docs: revert remove of "and"

0082680

Update: rewrite logic for node options

75bea25

platinumazure requested changes Feb 3, 2018

View reviewed changes

nicholaschuayunzhi added 2 commits February 3, 2018 13:32

Docs: Correct grammar for options

005032d

Chore: Correct grammar in JSDoc

fd1a780

ilyavolodin approved these changes Feb 10, 2018

View reviewed changes

platinumazure approved these changes Feb 10, 2018

View reviewed changes

ilyavolodin merged commit 47ac478 into eslint:master Feb 10, 2018

renovate bot mentioned this pull request Feb 16, 2018

Update dependency eslint to v4 Urigo/tortilla#62

Merged

renovate bot mentioned this pull request Mar 23, 2018

Update eslint to 4.19.1 sbardian/DebtTrackerAPI#33

Merged

eslint-deprecated bot locked and limited conversation to collaborators Aug 10, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: add named imports and exports for object-curly-newline #9876

Update: add named imports and exports for object-curly-newline #9876

nicholaschuayunzhi commented Jan 23, 2018

jsf-clabot commented Jan 23, 2018 •

edited

nicholaschuayunzhi commented Jan 29, 2018

platinumazure commented Jan 29, 2018

platinumazure left a comment

platinumazure Feb 2, 2018

platinumazure Feb 2, 2018

nicholaschuayunzhi Feb 3, 2018

platinumazure Feb 2, 2018

platinumazure Feb 2, 2018

platinumazure Feb 2, 2018

nicholaschuayunzhi Feb 3, 2018

platinumazure Feb 2, 2018

nicholaschuayunzhi Feb 3, 2018

platinumazure Feb 3, 2018

nicholaschuayunzhi commented Feb 3, 2018

platinumazure left a comment

platinumazure Feb 3, 2018

nicholaschuayunzhi commented Feb 5, 2018

ilyavolodin commented Feb 10, 2018

platinumazure left a comment

nicholaschuayunzhi commented Feb 10, 2018

Update: add named imports and exports for object-curly-newline #9876

Update: add named imports and exports for object-curly-newline #9876

Conversation

nicholaschuayunzhi commented Jan 23, 2018

jsf-clabot commented Jan 23, 2018 • edited

nicholaschuayunzhi commented Jan 29, 2018

platinumazure commented Jan 29, 2018

platinumazure 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

Explanation:

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

nicholaschuayunzhi commented Feb 3, 2018

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaschuayunzhi commented Feb 5, 2018

ilyavolodin commented Feb 10, 2018

platinumazure left a comment

Choose a reason for hiding this comment

nicholaschuayunzhi commented Feb 10, 2018

jsf-clabot commented Jan 23, 2018 •

edited