angular / clang-format

Node repackaging of the clang-format native binary
Apache License 2.0
98 stars 32 forks source link

Abusive use of indentation #20

Closed Foxandxss closed 9 years ago

Foxandxss commented 9 years ago

Hello,

We are using this library on ng-bootstrap and we are seeing an abusive use of indentation in our tests:

it('should show 10 stars by default', injectAsync([TestComponentBuilder], (tcb) => {
     return tcb.createAsync(NgbRating).then((fixture) => {
       fixture.detectChanges();

       const compiled = fixture.debugElement.nativeElement;

       const stars = getStars(compiled);
       expect(stars.length).toBe(10);
     });
   }));

I guess it is due to having a callback which has another callback in it.

Any ideas? Thanks.

mprobst commented 9 years ago

@Foxandxss clang-format tries to have indentation express your code structure. In this case, you have two levels of parameter nesting - one for the function call to it, and then another one for the call to injectAsync. I think this is working as intended in that regard.

That being said, what about having a helper function that wraps these together? I think it(..., injectAsync(... is common enough to warrant that:

function itIj(msg: string, ...componentOrFn: Function[]) {
  let injectables = componentOrFn.slice(0, -1);
  let testFn = componentOrFn.slice(-1)[0];
  it(msg, injectables, testFn);
}
Foxandxss commented 9 years ago

That should go into Angular's core I guess, but I don't think they are willing to.

Thank you.

mprobst commented 9 years ago

Have you tried asking about that? I think that's a completely sensible request, and I happen to be on the core team ;-)

Maybe file an issue with Angular2?

Foxandxss commented 9 years ago

I will do, ng2 testing needs lot of sugar, but I am not really fond of writing code to workaround formatters.

mprobst commented 9 years ago

That's certainly a matter of perspective, but note that the formatter actually formats your code correctly according to its structure, it's your code that generates a lot of indentation here. Introducing such a function simplifies the code, including its nesting depth.

wesleycho commented 9 years ago

I would like to request a reconsideration here - the indentation not only is odd given typical JS conventions, but it also is indenting an odd number of spaces. This makes it so that if someone has to edit the file in the future, a tab will indent 1 space instead of the expected 2 spaces, which is really frustrating when indentation structure helps when writing code to keep an eye on structure.

In particular, the situation @Foxandxss quoted, it is indenting at 3, 5, and 7 spaces, not even at 4, 6, and 8 even if this was expected behavior.

The suggested solution requires added code to work around what is by far a prevalent code style, which is an enforcement of code style which most would probably disagree with as proper behavior here. IMO, this should not be going so far as to enforce code style on this level.