Add trailing spaces presubmit check #5733

liyuqian · 2018-07-12T22:39:07Z

So we'll no longer need #5734 and #5708

gspencergoog · 2018-07-12T23:16:15Z

travis/format.sh

+
+for f in $FILES_TO_CHECK; do
+  set +e
+  TRAILING_SPACES="$(grep -P " +$" $f)"


I'd look for trailing tab characters too.

Just use \s to match all whitespace.

Also, use single quotes in the grep invocation in the subshell, and add double quotes around the "$f".

And I'd use "-E" instead of "-P", since that doesn't exist on macOS grep: you can use the blank character class too (which is space and tab).

grep -E '[[:blank:]]+$' .

You can avoid both -E and -P with \s unless [[:blank:]] matches a superset of \s.

Done. Thanks for the suggestion!

cbracken · 2018-07-12T23:34:18Z

travis/format.sh

+
+if [[ $FAILED_CHECKS -ne 0 ]]; then
+  echo ""
+  echo "ERROR: Some files have trailing spaces. To fix, try something like \`find . -name "*.dart" -print0 | xargs -0 perl -pi -e 's/ +$//'\`."


Patch up as described in person, should be significantly shorter too :)

gspencergoog · 2018-07-13T01:04:40Z

travis/format.sh

 for f in $FILES_TO_CHECK; do
  set +e
-  TRAILING_SPACES="$(grep -P " +$" $f)"
+  TRAILING_SPACES="$(grep '\s\+$' $f)"


Note that \s also matches return, vertical tab, form feed and, technically, newline. I'd just use [[:blank:]], since that just matches actual space and tab characters, and the others, while we probably want to remove them if they're in the code, it's more likely that they're intentionally there as part of a """ string.

Oh, also \s (which is a synonym for [[:space:]]) is locale dependent and will match any whitespace characters in the locale, and those (besides space and tab) are even more likely to be intentional, IMO.

But if we're sure we want to eliminate ALL whitespace at EOL, then I'm fine with \s, since there are workarounds if you really want whitespace at EOL in a """ string anyhow (which now that I think about it more is probably a bad practice in any case).

Yeah, ignore me. Listen to Chris. :-)

gspencergoog · 2018-07-13T01:08:49Z

FYI, I made a PR for a similar check for the flutter repo: flutter/flutter#19329

liyuqian · 2018-07-13T17:52:20Z

@cbracken : please review the changed the git diff line as we discussed offline.

gspencergoog · 2018-07-13T18:24:38Z

travis/format.sh

+FILETYPES="*.c *.cc *.cpp *.h *.m *.mm *.dart"
+FILES_TO_CHECK="$(git diff $DIFF_OPTS -- master $FILETYPES)"
+
+for f in $FILES_TO_CHECK; do


I'd actually suggest you do this with xargs: it results in fewer invocations of grep (so it's marginally faster), it's more compact, and it avoids problems where the command line size limit is reached.

TRAILING_SPACES=$(git diff $DIFF_OPTS master -- $FILETYPES | xargs grep --line-number --with-filename '\s\+$')

gspencergoog · 2018-07-13T18:25:53Z

travis/format.sh

+
+for f in $FILES_TO_CHECK; do
+  set +e
+  TRAILING_SPACES="$(grep --line-number '\s\+$' $f)"


You'll also want to add --with-filename so that the grep line includes the filename.

Previously, the git diff always returns empty because everything after `--` is considered <path>. (So master is parsed as <path> instead of <branch>.)

Otherwise, the script will exit with error if grep finds no match.

liyuqian · 2018-07-13T20:43:40Z

@gspencergoog : xargs do have different exit code between linux and mac, and error code 123 only exists on linux. Anyway, set +e would solve all these problems and we've already used that in this script during earlier diff.

gspencergoog · 2018-07-13T20:45:34Z

Works for me. LGTM (again).

This reverts commit a28c3e7.

chinmaygarde · 2018-07-26T01:18:32Z

This is giving me unexpected results on https://travis-ci.org/flutter/engine/builds/408311343?utm_source=github_status&utm_medium=notification. I have run clang-format over the file to make sure. Also, why are we reinventing clang-format? Both #5734 and #5708 would have been caught if we had ensured that clang-format was run over the diff. There is also a python script that performs clang-format over just the diff instead of over the entire source tree.

liyuqian · 2018-07-26T02:51:07Z

We don't run clang-format for dart so this guards dart. I think there might be some windows new lines in path_win.cc and that's why it failed this script? The `sed` command should fix that.

…

On Wed, Jul 25, 2018, 6:18 PM Chinmay Garde ***@***.***> wrote: This is giving me unexpected results on https://travis-ci.org/flutter/engine/builds/408311343?utm_source=github_status&utm_medium=notification. I have run clang-format over the file to make sure. Also, why are we reinventing clang-format? Both #5734 <#5734> and #5708 <#5708> would have been caught if we had ensured that clang-format was run over the diff. There is also a python script that performs clang-format over just the diff instead of over the entire source tree. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AV7DMPMuViYeeCdmJpeXAXFOWegwmBHUks5uKRjtgaJpZM4VN8uZ> .

chinmaygarde · 2018-07-26T03:09:46Z

Can we just run this on Dart files then? I'll try the sed command again. What's weird is that I was editing that file on my Mac. Chinmay

…

On Wed, Jul 25, 2018 at 7:51 PM liyuqian ***@***.***> wrote: We don't run clang-format for dart so this guards dart. I think there might be some windows new lines in path_win.cc and that's why it failed this script? The `sed` command should fix that. On Wed, Jul 25, 2018, 6:18 PM Chinmay Garde ***@***.***> wrote: > This is giving me unexpected results on > https://travis-ci.org/flutter/engine/builds/408311343?utm_source=github_status&utm_medium=notification . > I have run clang-format over the file to make sure. Also, why are we > reinventing clang-format? Both #5734 > <#5734> and #5708 > <#5708> would have been caught if > we had ensured that clang-format was run over the diff. There is also a > python script that performs clang-format over just the diff instead of over > the entire source tree. > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#5733 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AV7DMPMuViYeeCdmJpeXAXFOWegwmBHUks5uKRjtgaJpZM4VN8uZ > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACsNd3OKNafNOOhFF2fLMGdkvXKJAwwks5uKS6dgaJpZM4VN8uZ> .

liyuqian · 2018-07-26T03:25:19Z

Confirmed that there were CLRF new lines: #5871 Those probably are not caused by your Mac, but by the original author who wrote the code on Windows. Considering this new line issue that the clang-format doesn't catch, maybe we should leave this script as is? On Wed, Jul 25, 2018 at 8:09 PM Chinmay Garde <notifications@github.com> wrote:

…

Can we just run this on Dart files then? I'll try the sed command again. What's weird is that I was editing that file on my Mac. Chinmay On Wed, Jul 25, 2018 at 7:51 PM liyuqian ***@***.***> wrote: > We don't run clang-format for dart so this guards dart. I think there might > be some windows new lines in path_win.cc and that's why it failed this > script? The `sed` command should fix that. > > On Wed, Jul 25, 2018, 6:18 PM Chinmay Garde ***@***.***> > wrote: > > > This is giving me unexpected results on > > > https://travis-ci.org/flutter/engine/builds/408311343?utm_source=github_status&utm_medium=notification > . > > I have run clang-format over the file to make sure. Also, why are we > > reinventing clang-format? Both #5734 > > <#5734> and #5708 > > <#5708> would have been caught if > > we had ensured that clang-format was run over the diff. There is also a > > python script that performs clang-format over just the diff instead of > over > > the entire source tree. > > > > — > > You are receiving this because you modified the open/close state. > > Reply to this email directly, view it on GitHub > > <#5733 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/AV7DMPMuViYeeCdmJpeXAXFOWegwmBHUks5uKRjtgaJpZM4VN8uZ > > > > . > > > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > <#5733 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AACsNd3OKNafNOOhFF2fLMGdkvXKJAwwks5uKS6dgaJpZM4VN8uZ > > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AV7DMPu_taPQLT9uS20vJWkilcjrHptQks5uKTMCgaJpZM4VN8uZ> .

chinmaygarde · 2018-07-26T03:32:07Z

I am not sure this was really an issue. I'd rather have only one tool that enforces formatting since it is easier to integrate into code editors and lots of IDEs already have clang-format support built in. So from a development perspective, there isn't much to do to get started. Now, if you feel strongly about having uniform newlines, I am sure you can patch clang-format itself. Similarly, for Dart code, I am sure patching the Dart formatter to deal with newlines is the way to go.

…

On Wed, Jul 25, 2018 at 8:25 PM liyuqian ***@***.***> wrote: Confirmed that there were CLRF new lines: #5871 Those probably are not caused by your Mac, but by the original author who wrote the code on Windows. Considering this new line issue that the clang-format doesn't catch, maybe we should leave this script as is? On Wed, Jul 25, 2018 at 8:09 PM Chinmay Garde ***@***.***> wrote: > Can we just run this on Dart files then? I'll try the sed command again. > What's weird is that I was editing that file on my Mac. > > Chinmay > > On Wed, Jul 25, 2018 at 7:51 PM liyuqian ***@***.***> wrote: > > > We don't run clang-format for dart so this guards dart. I think there > might > > be some windows new lines in path_win.cc and that's why it failed this > > script? The `sed` command should fix that. > > > > On Wed, Jul 25, 2018, 6:18 PM Chinmay Garde ***@***.***> > > wrote: > > > > > This is giving me unexpected results on > > > > > > https://travis-ci.org/flutter/engine/builds/408311343?utm_source=github_status&utm_medium=notification > > . > > > I have run clang-format over the file to make sure. Also, why are we > > > reinventing clang-format? Both #5734 > > > <#5734> and #5708 > > > <#5708> would have been caught > if > > > we had ensured that clang-format was run over the diff. There is also a > > > python script that performs clang-format over just the diff instead of > > over > > > the entire source tree. > > > > > > — > > > You are receiving this because you modified the open/close state. > > > Reply to this email directly, view it on GitHub > > > <#5733 (comment) >, > > or mute > > > the thread > > > < > > > https://github.com/notifications/unsubscribe-auth/AV7DMPMuViYeeCdmJpeXAXFOWegwmBHUks5uKRjtgaJpZM4VN8uZ > > > > > > . > > > > > > > — > > You are receiving this because your review was requested. > > Reply to this email directly, view it on GitHub > > <#5733 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/AACsNd3OKNafNOOhFF2fLMGdkvXKJAwwks5uKS6dgaJpZM4VN8uZ > > > > . > > > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#5733 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AV7DMPu_taPQLT9uS20vJWkilcjrHptQks5uKTMCgaJpZM4VN8uZ > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#5733 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACsNbqlA7wjtx4J6NVRVpjZEbEHKctGks5uKTaggaJpZM4VN8uZ> .

liyuqian · 2018-07-26T18:02:52Z

Thanks @chinmaygarde for the feedback! I agree that using a single tool such as clang-format is a better way to go. I guess that we don't clang-format dart now because most of our dart files don't comply with clang-format. So if we're using clang-format for dart, we have to disable every other check except trailing spaces (and potentially new lines if we choose to do so).

gspencergoog · 2018-07-27T18:47:55Z

@liyuqian, clang-format doesn't format Dart files, it formats C/C++ (clang => C-Language). We don't run dartfmt because our style guide doesn't conform with the output of dartfmt.

liyuqian · 2018-07-27T23:03:52Z

@gspencergoog it does format Dart file for me: ../buildtools/linux-x64/clang/bin/clang-format lib/ui/painting.dart. Maybe our repo has special clang-format config?

liyuqian requested review from cbracken, chinmaygarde and jason-simmons July 12, 2018 22:39

googlebot added the cla: yes label Jul 12, 2018

gspencergoog reviewed Jul 12, 2018

View reviewed changes

cbracken reviewed Jul 12, 2018

View reviewed changes

gspencergoog reviewed Jul 13, 2018

View reviewed changes

liyuqian mentioned this pull request Jul 13, 2018

Removes trailing whitespace from source files. flutter/flutter#19329

Merged

gspencergoog reviewed Jul 13, 2018

View reviewed changes

liyuqian added 7 commits July 13, 2018 13:04

Add trailing spaces presubmit check

bd9944e

Match more whit spaces (e.g., tab)

24d789b

Fix the git diff

082ebd1

Previously, the git diff always returns empty because everything after `--` is considered <path>. (So master is parsed as <path> instead of <branch>.)

Add line number

c40e31d

Add filename

20545d1

Use xargs

d037de3

Skip the error code

f21dece

Otherwise, the script will exit with error if grep finds no match.

liyuqian force-pushed the whitespace2 branch from 1ae57c1 to f21dece Compare July 13, 2018 20:41

liyuqian merged commit a28c3e7 into flutter:master Jul 13, 2018

jonahwilliams pushed a commit that referenced this pull request Jul 13, 2018

Revert "Add trailing spaces presubmit check (#5733)"

a06fba0

This reverts commit a28c3e7.

liyuqian deleted the whitespace2 branch September 12, 2018 22:51

Add trailing spaces presubmit check #5733

Add trailing spaces presubmit check #5733

Uh oh!

Conversation

liyuqian commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gspencergoog commented Jul 13, 2018

Uh oh!

liyuqian commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyuqian commented Jul 13, 2018

Uh oh!

gspencergoog commented Jul 13, 2018

Uh oh!

chinmaygarde commented Jul 26, 2018

Uh oh!

liyuqian commented Jul 26, 2018 via email

Uh oh!

chinmaygarde commented Jul 26, 2018 via email

Uh oh!

liyuqian commented Jul 26, 2018 via email

Uh oh!

chinmaygarde commented Jul 26, 2018 via email

Uh oh!

liyuqian commented Jul 26, 2018

Uh oh!

gspencergoog commented Jul 27, 2018

Uh oh!

liyuqian commented Jul 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

liyuqian commented Jul 12, 2018 •

edited

Loading

cbracken Jul 12, 2018 •

edited

Loading

cbracken Jul 12, 2018 •

edited

Loading

gspencergoog Jul 13, 2018 •

edited

Loading

gspencergoog Jul 13, 2018 •

edited

Loading