Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't reformat regexp /litterals/ #16

Closed
vicb opened this issue Jun 23, 2015 · 18 comments
Closed

don't reformat regexp /litterals/ #16

vicb opened this issue Jun 23, 2015 · 18 comments
Assignees
Labels

Comments

@vicb
Copy link
Contributor

vicb commented Jun 23, 2015

see angular/angular#2695

/cc @mprobst

@vicb vicb added the bug label Jun 23, 2015
@vicb
Copy link
Contributor Author

vicb commented Jun 23, 2015

The failing code is available in https://github.com/vicb/angular/tree/0623-regexp.fail

@mprobst
Copy link
Contributor

mprobst commented Jun 23, 2015

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

@vicb
Copy link
Contributor Author

vicb commented Jun 23, 2015

@mprobst
Copy link
Contributor

mprobst commented Jun 23, 2015

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
Copy link
Contributor Author

vicb commented Jun 23, 2015

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

@vicb
Copy link
Contributor Author

vicb commented Jun 23, 2015

And that should be it.

@vicb
Copy link
Contributor Author

vicb commented Jul 1, 2015

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
Copy link
Contributor

mprobst commented Jul 2, 2015

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

@vicb
Copy link
Contributor Author

vicb commented Jul 2, 2015

// 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
Copy link
Contributor

mprobst commented Jul 2, 2015

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
Copy link
Contributor Author

vicb commented Jul 2, 2015

These all seem to work fine

What do you mean ???

@mprobst
Copy link
Contributor

mprobst commented Jul 2, 2015

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
Copy link
Contributor Author

vicb commented Jul 2, 2015

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

@vicb
Copy link
Contributor Author

vicb commented Jul 2, 2015

npm ing...

@vicb
Copy link
Contributor Author

vicb commented Jul 2, 2015

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
Copy link
Contributor Author

vicb commented Jul 2, 2015

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
Copy link
Contributor

mprobst commented Jul 2, 2015

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
Copy link
Contributor

mprobst commented Jul 2, 2015

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

@mprobst mprobst closed this as completed Jul 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants