angular/clang-format

don't reformat regexp /litterals/

vicb opened this issue · 18 comments

mprobst commented

Could you give a short example of the code that fails? It's a bit hard to dig out of an entire Angular tree.

mprobst commented

So I find two issues.

Wraps before }:

var re = /polyfill-next-selector[^+}]*content:[\s]*?['"](.*?)['"][;\s]*}([^{]*?){/gim;

Trailing /g screws up subsequent expressions:

var x = / "/g;
        foo();

If there's anything else wrong, please paste the actual failing regular expression into this issue; it's too hard to find some incorrect formatting in the messy github diff with lots of other changes.

vicb commented

var x = / "/g; shoud be var x = /"/g; (no leading space)

vicb commented

And that should be it.

vicb commented

There is actually one more occurrence now with the latest clang.

Look for TODO(vicb): see https://github.com/angular/clang-format/issues/16 in the PR

mprobst commented

Could you just post the code that fails to format here @vicb?

vicb commented 
// TODO(vicb): see https://github.com/angular/clang-format/issues/16
// clang-format off
var _cssContentNextSelectorRe =
    /polyfill-next-selector[^}]*content:[\s]*?['"](.*?)['"][;\s]*}([^{]*?){/gim;
 var _cssContentRuleRe =
    RegExpWrapper.create('(polyfill-rule)[^}]*(content:[\\s]*[\'"](.*?)[\'"])[;\\s]*[^}]*}', 'im');
var _cssContentUnscopedRuleRe = RegExpWrapper.create(
    '(polyfill-unscoped-rule)[^}]*(content:[\\s]*[\'"](.*?)[\'"])[;\\s]*[^}]*}', 'im');
    /(polyfill-rule)[^}]*(content:[\s]*['"](.*?)['"])[;\s]*[^}]*}/gim;
var _cssContentUnscopedRuleRe =
  /(polyfill-unscoped-rule)[^}]*(content:[\s]*['"](.*?)['"])[;\s]*[^}]*}/gim;
// clang-format on

// TODO(vicb): see https://github.com/angular/clang-format/issues/16
// clang-format off
/\/deep\//g,         // former >>>
/\/shadow-deep\//g,  // former /deep/
/\/shadow\//g,       // former ::shadow
// clanf-format on

// TODO(vicb): see https://github.com/angular/clang-format/issues/16 -> /"/g
// clang-format off
private static _quoteRegExp = /["]/g;
// clang-format on
mprobst commented

These all seem to work fine, with the sole exception of having a RegExp as a top level expression, e.g.

/foo/g.test('foo');

But that code is never useful, so I think we should just ignore it for the moment.

vicb commented

These all seem to work fine

What do you mean ???

mprobst commented

If I enable clang-formatting by removing the comments and format the code above with clang-format 1.0.25, the result is ok, except for the top level regexp expressions.

Or am I missing something?

vicb commented

Let me re-check but the one in the middle is new after having rebased yesterday.

vicb commented

npm ing...

vicb commented

The new one still fails (former are fine now)

$ gulp enforce-format
Dart SDK detected:
[11:27:47] Using gulpfile ~/dart/angular/gulpfile.js
[11:27:47] Starting 'enforce-format'...
[11:28:03] WARNING: Files are not properly formatted. Please run
[11:28:03]   ./node_modules/clang-format/index.js -i -style="file" /home/victor/dart/angular/modules/angular2/src/render/dom/shadow_dom/shadow_css.ts
[11:28:03]   (using clang-format version 1.0.25)

->

-  /\/deep\//g,         // former >>>
-  /\/shadow-deep\//g,  // former /deep/
-  /\/shadow\//g,       // former ::shadow
+  /\/deep\// g,           // former >>>
+  /\/shadow - deep\// g,  // former /deep/
+  /\/shadow\// g,         // former ::shadow

shadow - deep, no it's not a minus op

vicb commented

Original code is:

var _shadowDOMSelectorsRe = [
  />>>/g,
  /::shadow/g,
  /::content/g,
  // Deprecated selectors
  // TODO(vicb): see https://github.com/angular/clang-format/issues/16
  // clang-format off
  /\/deep\//g,         // former >>>
  /\/shadow-deep\//g,  // former /deep/
  /\/shadow\//g,       // former ::shadow
  // clanf-format on
];
mprobst commented

So the actual code that fails formatting is (without the // clang-format off):

var _shadowDOMSelectorsRe = [
  />>>/g,
  /::shadow/g,
  /::content/g,
  /\/deep\//g,         // former >>>
  /\/shadow-deep\//g,  // former /deep/
  /\/shadow\//g,       // former ::shadow
];
mprobst commented

I just released clang-format v1.0.26 that should fix this.