angular/clang-format

flip-flop ! (template string formatting is unstable)

vicb opened this issue · 8 comments

vicb commented

flip:

  compile(template: ViewDefinition): Promise<ProtoViewDto> {
    var tplPromise = this._templateLoader.load(template);
    return PromiseWrapper.then(
        tplPromise, (el) => this._compileTemplate(template, el, ProtoViewDto.COMPONENT_VIEW_TYPE),
        (e) => {
          throw new BaseException(`Failed to load the template for "${template.componentId}": ${e}`);
        });
  }

flop:

  compile(template: ViewDefinition): Promise<ProtoViewDto> {
    var tplPromise = this._templateLoader.load(template);
    return PromiseWrapper.then(
        tplPromise, (el) => this._compileTemplate(template, el, ProtoViewDto.COMPONENT_VIEW_TYPE),
        (e) => {
          throw new BaseException(
              `Failed to load the template for "${template.componentId}": ${e}`);
        });
  }

... infinite loop !

as it changes in every single run, the format check also fails on every single run !

/cc @mprobst

It's miscounting the length of the template string. I'll look into it. For now, the workaround is:

class X {
  compile(template: ViewDefinition): Promise<ProtoViewDto> {
    var tplPromise = this._templateLoader.load(template);
    return PromiseWrapper.then(
        tplPromise, (el) => this._compileTemplate(template, el, ProtoViewDto.COMPONENT_VIEW_TYPE),
        (e) => {
          var msg = `Failed to load the template for "${template.componentId}": ${e}`;
          throw new BaseException(msg);
        });
  }
}
vicb commented

I have "fixed" this by adding a space before the ':'

vicb commented

Thanks for looking into this.

to consider: we could improve the error reporting here by double-formatting inside gulp-clang-format, to ensure that the proposed formatting change is stable. If we find that the second formatting pass produces a different answer, we could silence the error for that file.
I believe we've seen a few different bugs manifest in this way, so perhaps this is not the last one either?

@alexeagle I'll see if I can do something about this, this happens more often than I thought. I don't think changing gulp-clang-format to run twice is a good idea; this is just a bug in clang-format that should be fixed.

vicb commented

this is just a bug in clang-format that should be fixed.

👍

I've just released v1.0.26 that should fix these issues for template literals.

vicb commented

Thanks !