Issue 7151043: Adding CheckChangeHasOnlyOneEol to upload and commit presubmit checks (Closed)
|
Created:
11 years, 11 months ago by rmistry Modified:
11 years, 10 months ago CC:
skia-review_googlegroups.com, skiabot_google.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdding CheckChangeHasOnlyOneEol to upload and commit presubmit checks
Committed: https://code.google.com/p/skia/source/detail?r=7247
Patch Set 1 #
Total comments: 1
Patch Set 2 : #MessagesTotal messages: 12
https://codereview.appspot.com/7151043/diff/1/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.appspot.com/7151043/diff/1/PRESUBMIT.py#newcode16 PRESUBMIT.py:16: x.LocalPath().endswith('.cpp')) I guess it's not as critical since it won't break the build, but it might be nice to also check .gyp, .gypi, and .py. Possibly even shell scripts?
Sign in to reply to this message.
LGTM but I defer to Eric
Sign in to reply to this message.
On 2013/01/17 14:29:22, EricB wrote: > https://codereview.appspot.com/7151043/diff/1/PRESUBMIT.py > File PRESUBMIT.py (right): > > https://codereview.appspot.com/7151043/diff/1/PRESUBMIT.py#newcode16 > PRESUBMIT.py:16: x.LocalPath().endswith('.cpp')) > I guess it's not as critical since it won't break the build, but it might be > nice to also check .gyp, .gypi, and .py. Possibly even shell scripts? Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/17 14:42:27, EricB wrote: > LGTM This just kicked in for me. I had two newlines at the bottom of a few files. It's great but would it be much harder to have the script offer to fix the offending files?
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/17 19:39:01, bsalomon wrote: > On 2013/01/17 14:42:27, EricB wrote: > > LGTM > > This just kicked in for me. I had two newlines at the bottom of a few files. > It's great but would it be much harder to have the script offer to fix the > offending files? We could, it may get a little complicated though, not sure its worth the effort since it is such an easy fix for the developer :)
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/17 19:45:11, rmistry wrote: > On 2013/01/17 19:39:01, bsalomon wrote: > > On 2013/01/17 14:42:27, EricB wrote: > > > LGTM > > > > This just kicked in for me. I had two newlines at the bottom of a few files. > > It's great but would it be much harder to have the script offer to fix the > > offending files? > > We could, it may get a little complicated though, not sure its worth the effort > since it is such an easy fix for the developer :) I'm getting lots of warnings about extra newlines in files I've edited where the extra newlines pre-date my change. Can we do one of the following: 1) only check that there is at least one newline (rather than exactly one) or 2) make the nightly script fix the multi-newline violations that predate this checker (or are introduced by people not using gcl)?
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/25 16:16:24, bsalomon wrote: > On 2013/01/17 19:45:11, rmistry wrote: > > On 2013/01/17 19:39:01, bsalomon wrote: > > > On 2013/01/17 14:42:27, EricB wrote: > > > > LGTM > > > > > > This just kicked in for me. I had two newlines at the bottom of a few files. > > > It's great but would it be much harder to have the script offer to fix the > > > offending files? > > > > We could, it may get a little complicated though, not sure its worth the > effort > > since it is such an easy fix for the developer :) > > I'm getting lots of warnings about extra newlines in files I've edited where the > extra newlines pre-date my change. Can we do one of the following: > > 1) only check that there is at least one newline (rather than exactly one) or > > 2) make the nightly script fix the multi-newline violations that predate this > checker (or are introduced by people not using gcl)? I can do either one, maybe (2) is the better fix?
Sign in to reply to this message.
SGTM On Fri, Jan 25, 2013 at 11:30 AM, <rmistry@google.com> wrote: > On 2013/01/25 16:16:24, bsalomon wrote: > >> On 2013/01/17 19:45:11, rmistry wrote: >> > On 2013/01/17 19:39:01, bsalomon wrote: >> > > On 2013/01/17 14:42:27, EricB wrote: >> > > > LGTM >> > > >> > > This just kicked in for me. I had two newlines at the bottom of a >> > few files. > >> > > It's great but would it be much harder to have the script offer to >> > fix the > >> > > offending files? >> > >> > We could, it may get a little complicated though, not sure its worth >> > the > >> effort >> > since it is such an easy fix for the developer :) >> > > I'm getting lots of warnings about extra newlines in files I've edited >> > where the > >> extra newlines pre-date my change. Can we do one of the following: >> > > 1) only check that there is at least one newline (rather than exactly >> > one) or > > 2) make the nightly script fix the multi-newline violations that >> > predate this > >> checker (or are introduced by people not using gcl)? >> > > I can do either one, maybe (2) is the better fix? > > https://codereview.appspot.**com/7151043/<https://codereview.appspot.com/7151... >
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/25 16:35:02, bsalomon wrote: > SGTM > Ended up doing both: https://codereview.appspot.com/7216043/ https://codereview.appspot.com/7193063/ > > On Fri, Jan 25, 2013 at 11:30 AM, <mailto:rmistry@google.com> wrote: > > > On 2013/01/25 16:16:24, bsalomon wrote: > > > >> On 2013/01/17 19:45:11, rmistry wrote: > >> > On 2013/01/17 19:39:01, bsalomon wrote: > >> > > On 2013/01/17 14:42:27, EricB wrote: > >> > > > LGTM > >> > > > >> > > This just kicked in for me. I had two newlines at the bottom of a > >> > > few files. > > > >> > > It's great but would it be much harder to have the script offer to > >> > > fix the > > > >> > > offending files? > >> > > >> > We could, it may get a little complicated though, not sure its worth > >> > > the > > > >> effort > >> > since it is such an easy fix for the developer :) > >> > > > > I'm getting lots of warnings about extra newlines in files I've edited > >> > > where the > > > >> extra newlines pre-date my change. Can we do one of the following: > >> > > > > 1) only check that there is at least one newline (rather than exactly > >> > > one) or > > > > 2) make the nightly script fix the multi-newline violations that > >> > > predate this > > > >> checker (or are introduced by people not using gcl)? > >> > > > > I can do either one, maybe (2) is the better fix? > > > > > https://codereview.appspot.**com/7151043/%3Chttps://codereview.appspot.com/71...> > >
Sign in to reply to this message.
Message was sent while issue was closed.
On 2013/01/25 18:28:20, rmistry wrote: > On 2013/01/25 16:35:02, bsalomon wrote: > > SGTM > > > > Ended up doing both: > https://codereview.appspot.com/7216043/ > https://codereview.appspot.com/7193063/ > Great. Thanks, Ravi!
Sign in to reply to this message.
|
