Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upObject destructuring spacing #182
Comments
This comment has been minimized.
This comment has been minimized.
|
Yeah, I'm in favor of this if there's some way to get this rule just for object destructuring, i.e. not for object literals too: var x = { a: 1, b: 2 }I would like to enforce this spacing for object literals too, but at this point it's a huge breaking change and lots of people are doing: var x = {a: 1}I think eslint might have only one rule for object literals and object destructuring, but I can't remember. Want to look into it? |
feross
added
the
question
label
Jul 2, 2015
This comment has been minimized.
This comment has been minimized.
|
@feross are we able to use warnings as syntax deprecation messages? |
This comment has been minimized.
This comment has been minimized.
|
I suppose we could, but that's a departure from what we currently do. |
This comment has been minimized.
This comment has been minimized.
|
In an event where the above change would be introduced, I think the warning might be the best way to get an immediate, non-breaking discussion around its usage, if that is desired. |
This comment has been minimized.
This comment has been minimized.
|
What about just treating: var { x} = ...
var {x } = ...as errors. var { x } = ...
var {x} = ...would be allowed. Same for arrays. If I got Thoughts? |
This comment has been minimized.
This comment has been minimized.
|
100% agree, that is a great start and enforces some form of sane consistency. |
This comment has been minimized.
This comment has been minimized.
|
Yep, I like this too |
This comment has been minimized.
This comment has been minimized.
|
Does someone want to make an eslint issue for this? I don't think this is a rule option they currently support. |
This comment has been minimized.
This comment has been minimized.
dcousens
added
blocked
and removed
question
labels
Jul 8, 2015
This comment has been minimized.
This comment has been minimized.
|
@feross the feedback is that we should just use http://eslint.org/docs/rules/object-curly-spacing. |
This comment has been minimized.
This comment has been minimized.
|
@feross looks like it isn't going to change upstream, do we want to enforce a particular style then? |
This comment has been minimized.
This comment has been minimized.
This should happen a lot less going forward. I locked the eslint version so it's exact now. So if even a patch version of eslint starts causing new tests to fail, we can decide if we want to bump the major version of standard.
For any breaking style changes, even in a major, I want to make sure that Before we do that though, @xjamundx offered to write an eslint plugin that enforces the "either" rule we wanted from eslint. |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 14, 2015
|
I can help you build this. It will take like 15 minutes. Does standard already have its own custom rules or a |
This comment has been minimized.
This comment has been minimized.
|
We don't have any custom rules right now. Can you put custom rules into a |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 14, 2015
|
Yes that's an option, but I'd recommend at the very least you created a dedicated rules directory. To use custom rules with eslint you either need to specify the |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 14, 2015
|
Also you only care about destructuring spacing or all of the existing things found in |
This comment has been minimized.
This comment has been minimized.
|
Okay, how about we start with a rules directory and then we can pull it out into a module eventually. We care about "even spacing" for everything in the |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 14, 2015
|
Sure. I can do both. Give me a day or so! |
This comment has been minimized.
This comment has been minimized.
|
@xjamundx You're awesome! |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 16, 2015
|
so funny to me:
|
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 16, 2015
|
Working on this. 15 minutes was a bit of an understatement, but I'm on an |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 16, 2015
|
Gist of the rule is right here: function isEvenlySpaced(node, start, end) {
var expectedSpace = start[0].range[1] - start[1].range[0];
var endSpace = end[0].range[1] - end[1].range[0];
return endSpace === expectedSpace;
}When that fails it will say something like "Expected consistent spacing". I need to add more edge cases, but it's coming along! |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 16, 2015
|
Any thoughts on handling this: var x = {
a: 'b'
}What eslint does it essentially ignore anything with var x = {
a: 'b'
}
var x = {
a: 'b' }You think that's still good enough for now? I'll try to poke around a bit and see if I can work out something like if one is followed by a newline the other should proceed immediately after one. |
This comment has been minimized.
This comment has been minimized.
|
Excellent! Your idea to special case the newline condition is the right idea. But of course, even without that, the rule would be better that what we have now |
This comment has been minimized.
This comment has been minimized.
|
You can try |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 17, 2015
|
I'm going to see if I can integrate this one rule into the standard package first then work on the array rule later |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 17, 2015
|
Okay, so |
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 17, 2015
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 17, 2015
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 17, 2015
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 17, 2015
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 18, 2015
This comment has been minimized.
This comment has been minimized.
|
@xjamundx awesome work!
Should that be acceptable? Can we enforce |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 20, 2015
|
@dcousens the way that this rule is setup you're only enforcing consistent spacing for a given objectexpression or objectpattern, not consistent spacing for all objects everywhere. |
This comment has been minimized.
This comment has been minimized.
|
@xjamundx OK, and its not easy to change that consistency to |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 21, 2015
|
@dcousens. We certainly could do that if it's desired. |
This comment has been minimized.
This comment has been minimized.
|
@xjamundx would that interfere with multiline? var { x } = ...
var {x} = ...
var {
x,
y
} = ... |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 22, 2015
|
I see your point. It's up to 1 space. |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 22, 2015
|
Yeah that's easy. By the way I'm messing around with this idea (not publishing to npm): It's much easier to write out the test cases when they're in a plugin like this: |
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 22, 2015
xjamundx
pushed a commit
to xjamundx/standard
that referenced
this issue
Jul 23, 2015
This comment has been minimized.
This comment has been minimized.
knownasilya
commented
Jul 28, 2015
|
Would love warnings when there is inconsistency, e.g. var test = {hello: 'blah'}
var test2 = { hello: 'blah' }
console.log(test, test2)In the same project/file. Or just sticking with one, and bumping the major version. |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 28, 2015
|
file-level consistency is doable, but not currently supported. |
This comment has been minimized.
This comment has been minimized.
|
@xjamundx I'm merging your work on eslint-plugin-standard right now. Wouldn't @knownasilya's example be caught? |
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 28, 2015
|
The inconsistency is per instance not per file. Per file would be a minor change, we'd just need to keep track of the first style and compare against future styles. |
This comment has been minimized.
This comment has been minimized.
|
@xjamundx Ah. |
This comment has been minimized.
This comment has been minimized.
|
Okay, @xjamundx's changes are released as standard 5.0.0-7 and we depend on |
feross
closed this
Jul 28, 2015
This comment has been minimized.
This comment has been minimized.
xjamundx
commented
Jul 28, 2015
|
awesome! |
This comment has been minimized.
This comment has been minimized.
|
Continue conversation @ Object curlies: File/project–wide or standard? |
dcousens commentedJul 2, 2015
These cases probably shouldn't be allowed.
I'm in favour of what we've done elsewhere, enforce the spaces either side.
In any case, consistency, thoughts?