Open mismith opened 7 years ago
I'm sorry this has caused so much discussion; the reality is that (for whatever reason) the "good" example you mention in the beginning of your post is simply bad. It should be formatted like this:
// good
const leds = stage
.selectAll('.led')
.data(data)
.enter()
.append('svg:svg')
.classed('led', true)
.attr('width', (radius + margin) * 2)
.append('svg:g')
.attr('transform', `translate(${radius + margin},${radius + margin})`)
.call(tron.led);
and in actuality, this should be split up into many many separate variables - overusing chaining is bad for readability. As so, something like:
// good
const leds = stage.selectAll('.led');
leds.data(data);
const svg = leds.enter()
.append('svg:svg')
.classed('led', true)
.attr('width', (radius + margin) * 2);
const g = svg.append('svg:g');
g.attr('transform', `translate(${radius + margin},${radius + margin})`)
.call(tron.led);
@ljharb as you said, overusing chaining is bad for readability. There are various ways to improve the readability, though.
The D3JS community has very widely adopted the way proposed by the original author of the library, see for instance https://bost.ocks.org/mike/d3/workshop/#35.
Your fixed example would not work, by the way. The leds
will not have the enter
method when created with selectAll
. The enter
method is present in what's returned by the data
call.
It's precisely this kind of operations that change the selection that have made the D3JS community (and jQuery community too, as I've understood) adopt the variable width indents.
Are there plans to update the style guide to reflect this change @ljharb?
Sorry for my comment being slightly offtopic maybe, but I'm thinking that adding any indent with chaining is both making code harder to read and semantically unjust, as no deeper level is really reached.
My coworkers and I are having trouble deciding how best to indent chained function calls, covered in section 19.6 - Whitespace > Chains
It appears to me that one of the 'good' examples given doesn't actually pass ESLinting:
In particular, it complains about the indentation being 4 spaces instead of 2, for the 5 lines where that's the case.
What's more, even though it might technically be permitted, there is some weirdness in this example that we think should be elaborated upon and/or clarified. Here are two examples:
.data(data)
) is indented 4 spaces, which at first glance seems pretty weird since it's two gutters in from it's parent. It makes more sense if the first line were split into two, though, e.g.Overall, we had some pretty involved discussion around when and where to use which, and often it seemed to be difficult to reason about because the semantics of the indenting seemed to depend on the semantics of the methods being called, which is tough to be consistent about. There are different strategies to deal with this, but it would be nice if this guide had an opinion that we could rely upon, instead of having to debate the merits of each alternative.
For instance, in #19, the issue that seems to have introduced function chaining rules, @quirkyjack proposes that