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
Add object-curly-newline
rule
#367
Comments
I kind of like being able to specify quick objects on one line when the number of keys is relatively small. const foo = 1, bar = 2;
const object = {foo, baz}; I'm not currently a big fan of this rule at the moment. |
Official name: Object Literal Property Value Shorthand |
I agree with @jfmengels. I have even disabled it on some files at work because of those "issues". It would've been nice if we could define a maximum number of props in an object that are allowed on one line. |
But consistency and readability! :) Isn't readability better than slight convenience? I applied XO on many projects today and a lot of code became clearer when the object properties got on separate lines. That also improves diffing and reduces chances of merge conflicts. That being said, I'm open to reconsider if everyone feels it's wrong, but please try it out a little bit first. It might just be something you need to warm up to.
How do we enforce "relatively small"? What number is that? Can you guys provide some real-world examples of it getting in the way? Btw, I know it's confusing, but this rule is actually different from the one that enforces multiple properties on new lines. See: xojs/eslint-config-xo@7ff9158 and xojs/eslint-config-xo#23 |
Sure, no problem with trying it out.
If I had to write the rule now, I'd go for a character length limit (meaning the number of characters between foo({a: 1, b: 2}) // nice and readable IMO (at least readable enough)
foo({
a: 1,
b: 2
}) // Wouldn't do it, but I get that it could be considered more readable
foo({
someProperty: Object.assign(foo, {ok: 2}),
someProperty2: Object.assign(foo, {ok: 3})
}) // Definitely would do it
foo({a: 1, b: 2, c: 3, d: 4}) // starting to get crowded.
foo({a, b, c, d}) // Reasonably readable
foo({
a,
b,
c,
d
}) // Definitely overkill
foo({prettyLongPropertyName1, prettyLongPropertyName2}) // readable
foo({
prettyLongPropertyName1,
prettyLongPropertyName2
}) // might start considering it, but probably wouldn't do it on my own
foo({prettyLongPropertyName1, prettyLongPropertyName2, prettyLongPropertyName3}) // getting crowded...?
foo({prettyLongPropertyName1: someValue, prettyLongPropertyName2: foo(a, b, c), prettyLongPropertyName3: prettyLongPropertyName3}) // (╯°□°)╯︵ ┻━┻ tl;dr: I'm far from having hard science and hard opinions here. Don't mind trying it out a bit. I'd say that |
Let say we allowed two properties. I too have done a lot of
Linting is strict for a reason. Sometimes it affects convenience, but in the long run it's better for everyone. In addition to readability and diffing benefits, I must mention refactoring/changing. It's a lot easier to delete and add properties when they're on separate lines. For background: I've been thinking about this rule for a long time, as I was unsure about it too, but I've had so many PRs with cluttered code from inline objects that I decided to pull the trigger now. See eslint/eslint#6211 regarding ignoring object shorthands. I agree it doesn't make sense to put those on separate lines.
The |
Ah cool!
When it comes to linting for submitted PRs, you have a lot more experience and frustration than me ;) Let's try it out, but I'd love to see eslint/eslint#6211 come to fruition to cut a break for object shorthand. |
Ok, I've disabled the rule until we decide on something here and eslint/eslint#6211 is done. |
When I started using |
See: eslint/eslint#6251 |
Looks like I've been very happy with this config: 'object-curly-newline' : ['error', {
ObjectExpression : {
multiline : true,
minProperties : 2,
consistent : true
},
ObjectPattern : {
multiline : true,
minProperties : 4,
consistent : true
},
ImportDeclaration : {
multiline : true,
minProperties : 4,
consistent : true
},
ExportDeclaration : {
multiline : true,
minProperties : 2,
consistent : true
}
}], I wrote some tests to prove that it behaves nicely: |
I tried your above config now and Object.assign(stream, {isTTY: true, columns, rows}); to Object.assign(stream, {
isTTY: true, columns, rows
}); |
If you want to allow one-line object literals for objects with three properties, then change the |
No, I meant, it should have fixed it to: Object.assign(stream, {
isTTY: true,
columns,
rows
}); |
This rule only affects the newlines for the curly braces, not the properties. There is a separate rule for the latter. Edit: Nevermind, I see that was already discussed. I guess the real problem is |
A couple of ideas: Ok{name, email, subject, sign} // Only keys
{name: 'Alpha', email: 'beta@gamma.com'} // Not a long line Not ok{name, email, subject, sign: true} // Not "only keys" anymore
{name: 'Alpha the son of Thor Junior and Laquisha', email: 'beta@gamma.com'} // Line is too long |
PR welcome :) |
http://eslint.org/docs/rules/object-curly-newline
The above will enforce putting the contents of objects (everything inside the braces) on a newline unless there's only one property.
The main issues with this rule is that it then enforces that all objects with one property is inline
{foo: true}
. I think it would make more sense not to enforce anything then, as having it inline or not when only property depends on the situation.The second issue is that it also applies to object destructuring, where having multiple properties inline is quite ok.
const {foo, bar} = unicorn();
.The text was updated successfully, but these errors were encountered: