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 named imports and exports for object-curly-newline #9876

Merged

Conversation

@nicholaschuayunzhi
Copy link
Contributor

nicholaschuayunzhi commented Jan 23, 2018

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

This comment has been minimized.

Copy link

jsf-clabot commented Jan 23, 2018

CLA assistant check
All committers have signed the CLA.

@nicholaschuayunzhi nicholaschuayunzhi force-pushed the nicholaschuayunzhi:8501-import-export-config branch from 3b51073 to dcef6de Jan 23, 2018
@nicholaschuayunzhi

This comment has been minimized.

Copy link
Contributor Author

nicholaschuayunzhi commented Jan 29, 2018

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

@platinumazure platinumazure self-assigned this Jan 29, 2018
@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Jan 29, 2018

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

Copy link
Member

platinumazure left a comment

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!

@@ -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:

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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

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

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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

This comment has been minimized.

Copy link
@nicholaschuayunzhi

nicholaschuayunzhi Feb 3, 2018

Author Contributor

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 } }
  1. find options that are objects.
  2. find the options that have a string or object as its value.
closeBrace = sourceCode.getLastToken(node);
} else {
closeBrace = sourceCode.getTokenAfter(node.specifiers.slice(-1)[0], token => token.value === "}");

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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

)
);

let needsLinebreaks;

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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

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

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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.

This comment has been minimized.

Copy link
@nicholaschuayunzhi

nicholaschuayunzhi Feb 3, 2018

Author Contributor

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.

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)

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 2, 2018

Member

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

This comment has been minimized.

Copy link
@nicholaschuayunzhi

nicholaschuayunzhi Feb 3, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 3, 2018

Member

Thanks for checking!

@nicholaschuayunzhi

This comment has been minimized.

Copy link
Contributor Author

nicholaschuayunzhi commented Feb 3, 2018

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

Copy link
Member

platinumazure left a comment

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!

@@ -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:

This comment has been minimized.

Copy link
@platinumazure

platinumazure Feb 3, 2018

Member

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

This comment has been minimized.

Copy link
Contributor Author

nicholaschuayunzhi commented Feb 5, 2018

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

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Feb 10, 2018

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

Copy link
Member

platinumazure left a comment

LGTM, thanks for contributing!

@ilyavolodin ilyavolodin merged commit 47ac478 into eslint:master Feb 10, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@nicholaschuayunzhi

This comment has been minimized.

Copy link
Contributor Author

nicholaschuayunzhi commented Feb 10, 2018

Thanks for your guidance!

This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.