You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Avinash sridharan <av...@mesosphere.io> on 2016/01/13 21:15:37 UTC
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 13, 2016, 8:15 p.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
Diffs
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing
-------
Ran the modified mesos-style.py on the existing code-base and got the following errors:
rc/authentication/cram_md5/authenticatee.hpp:60: Should have a space between // and comment [whitespace/mesos-comments] [4]
2 src/authentication/cram_md5/authenticator.hpp:60: Should have a space between // and comment [whitespace/mesos-comments] [4]
3 src/authorizer/local/authorizer.hpp:82: Should have a space between // and comment [whitespace/mesos-comments] [4]
4 src/tests/containerizer/docker_containerizer_tests.cpp:921: Should have a space between // and comment [whitespace/mesos-comments] [4]
5 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp:104: Should have a space between // and comment [whitespace/mesos-comments] [4]
6 Total errors found: 1
Doesn't seem to give any false-positives. All the above errors do have a missing leading white-space.
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index 90d56b9..17fcbed 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2238,7 +2238,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
EXPECT_SOME(portResult1);
EXPECT_SOME(portResult2);
- // Verify that the ports retrieved from state.json are the ones that were set.
+ //Verify that the ports retrieved from state.json are the ones that were set.
EXPECT_EQ(JSON::Object(JSON::protobuf(*port1)), portResult1.get());
EXPECT_EQ(JSON::Object(JSON::protobuf(*port2)), portResult2.get());
And the git commit hook correctly failed it:
git commit -m "Checking the new hook"
Checking 1 files
src/tests/slave_tests.cpp:2241: Should have a space between // and comment [whitespace/mesos-comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114417
-----------------------------------------------------------
Bad patch!
Reviews applied: [41617]
Failed command: ./support/apply-review.sh -n -r 41617
Error:
2016-01-14 05:12:09 URL:https://reviews.apache.org/r/41617/diff/raw/ [1614/1614] -> "41617.patch" [1]
No files to lint
Error: Commit message summary (the first line) must not exceed 72 characters.
- Mesos ReviewBot
On Jan. 14, 2016, 1:59 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 1:59 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the modified mesos-style.py on the existing code-base and got the following errors:
> src/authentication/cram_md5/authenticatee.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
> src/authentication/cram_md5/authenticator.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
> src/authorizer/local/authorizer.hpp:84: Should have a space between // and comment [whitespace/comments] [4]
> src/tests/containerizer/docker_containerizer_tests.cpp:978: Should have a space between // and comment [whitespace/comments] [4]
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp:90: At least a single space is required between code and comments [whitespace/comments] [2]
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp:127: Should have a space between // and comment [whitespace/comments] [4]
> Doesn't seem to give any false-positives. All the above errors do have a missing leading white-space.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Jan. 14, 2016, 11:02 p.m., Michael Park wrote:
> > support/cpplint.py, line 2641
> > <https://reviews.apache.org/r/41617/diff/3/?file=1197251#file1197251line2641>
> >
> > Sorry for the follow-up review after a `ShipIt`, but it looks like we missed something here. The comment says: `# Allow one space for new scopes, two spaces otherwise:`. The "new scope" part seems to be in this line: `Match(r'^\s*{ //', line)`. Can we just remove this now?
Ran the support/mesos-styles.py after this change and found zero errors.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114602
-----------------------------------------------------------
On Jan. 15, 2016, 3:34 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 3:34 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114602
-----------------------------------------------------------
Ship it!
support/cpplint.py (lines 32 - 33)
<https://reviews.apache.org/r/41617/#comment175457>
I think we can highlight the __single__ rather than the 'at least', since 'at least' is not the modified part.
support/cpplint.py (line 2641)
<https://reviews.apache.org/r/41617/#comment175454>
Sorry for the follow-up review after a `ShipIt`, but it looks like we missed something here. The comment says: `# Allow one space for new scopes, two spaces otherwise:`. The "new scope" part seems to be in this line: `Match(r'^\s*{ //', line)`. Can we just remove this now?
- Michael Park
On Jan. 14, 2016, 10:51 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 10:51 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114609
-----------------------------------------------------------
Bad patch!
Reviews applied: [42325, 42326, 41617]
Failed command: ./support/apply-review.sh -n -r 41617
Error:
2016-01-14 23:46:44 URL:https://reviews.apache.org/r/41617/diff/raw/ [1943/1943] -> "41617.patch" [1]
No files to lint
Error: Commit message summary (the first line) must not exceed 72 characters.
- Mesos ReviewBot
On Jan. 14, 2016, 10:51 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 10:51 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Enabled whitespace-comments category in
mesos-style.py
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114811
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Jan. 15, 2016, 5:42 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 5:42 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Enabled whitespace-comments category in mesos-style.py
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the support/mesos-styles.py after this change and found zero errors.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Enabled whitespace-comments category in
mesos-style.py
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 5:42 p.m.)
Review request for mesos and Michael Park.
Summary (updated)
-----------------
Enabled whitespace-comments category in mesos-style.py
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Enabled whitespace-comments category in mesos-style.py
Diffs
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing
-------
Ran the support/mesos-styles.py after this change and found zero errors.
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 5:41 p.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description (updated)
-------
Enabled whitespace-comments category in mesos-style.py
Diffs
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing
-------
Ran the support/mesos-styles.py after this change and found zero errors.
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114662
-----------------------------------------------------------
Bad patch!
Reviews applied: [42325, 42326, 41617]
Failed command: ./support/apply-review.sh -n -r 41617
Error:
2016-01-15 05:20:35 URL:https://reviews.apache.org/r/41617/diff/raw/ [1972/1972] -> "41617.patch" [1]
No files to lint
Error: Commit message summary (the first line) must not exceed 72 characters.
- Mesos ReviewBot
On Jan. 15, 2016, 3:35 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 3:35 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the support/mesos-styles.py after this change and found zero errors.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 3:35 a.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
Diffs
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing (updated)
-------
Ran the support/mesos-styles.py after this change and found zero errors.
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 15, 2016, 3:34 a.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
Diffs (updated)
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing
-------
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 14, 2016, 10:51 p.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
Diffs (updated)
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing (updated)
-------
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Jan. 14, 2016, 8:46 p.m., Michael Park wrote:
> > Please update the `Summary` to be < 72 chars to make Reviewbot happy. Could you also submit a patch for the places that violate the rule we're enforcing here and make this patch depend on it? Again, so that this way Reviewbot will be happy and our CI will continue to pass.
>
> Avinash sridharan wrote:
> Re-indented to make comments fit in 72 lines.
>
> Michael Park wrote:
> Oh, I meant the `Summary` of this review: "Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments"
> The `Summary` becomes the first line of the commit, and there's a pre-commit hook to check that it's < 72 chars which fails Reviewbot:
>
> ```
> Error: Commit message summary (the first line) must not exceed 72 characters.
> ```
Ah got it. Was wondering why the reviewbot was still complaining since I had changed the commmit message itself. Will change it.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114563
-----------------------------------------------------------
On Jan. 15, 2016, 3:35 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 3:35 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the support/mesos-styles.py after this change and found zero errors.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
> On Jan. 14, 2016, 8:46 p.m., Michael Park wrote:
> > Please update the `Summary` to be < 72 chars to make Reviewbot happy. Could you also submit a patch for the places that violate the rule we're enforcing here and make this patch depend on it? Again, so that this way Reviewbot will be happy and our CI will continue to pass.
Re-indented to make comments fit in 72 lines.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114563
-----------------------------------------------------------
On Jan. 15, 2016, 3:34 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 3:34 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Michael Park <mp...@apache.org>.
> On Jan. 14, 2016, 8:46 p.m., Michael Park wrote:
> > Please update the `Summary` to be < 72 chars to make Reviewbot happy. Could you also submit a patch for the places that violate the rule we're enforcing here and make this patch depend on it? Again, so that this way Reviewbot will be happy and our CI will continue to pass.
>
> Avinash sridharan wrote:
> Re-indented to make comments fit in 72 lines.
Oh, I meant the `Summary` of this review: "Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments"
The `Summary` becomes the first line of the commit, and there's a pre-commit hook to check that it's < 72 chars which fails Reviewbot:
```
Error: Commit message summary (the first line) must not exceed 72 characters.
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114563
-----------------------------------------------------------
On Jan. 15, 2016, 3:35 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 3:35 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the support/mesos-styles.py after this change and found zero errors.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/#review114563
-----------------------------------------------------------
Ship it!
Please update the `Summary` to be < 72 chars to make Reviewbot happy. Could you also submit a patch for the places that violate the rule we're enforcing here and make this patch depend on it? Again, so that this way Reviewbot will be happy and our CI will continue to pass.
support/cpplint.py (lines 31 - 32)
<https://reviews.apache.org/r/41617/#comment175408>
Could you add your name and a brief description of the modification here?
- Michael Park
On Jan. 14, 2016, 1:59 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41617/
> -----------------------------------------------------------
>
> (Updated Jan. 14, 2016, 1:59 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4231
> https://issues.apache.org/jira/browse/MESOS-4231
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
>
>
> Diffs
> -----
>
> support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
> support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
>
> Diff: https://reviews.apache.org/r/41617/diff/
>
>
> Testing
> -------
>
> Ran the modified mesos-style.py on the existing code-base and got the following errors:
> src/authentication/cram_md5/authenticatee.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
> src/authentication/cram_md5/authenticator.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
> src/authorizer/local/authorizer.hpp:84: Should have a space between // and comment [whitespace/comments] [4]
> src/tests/containerizer/docker_containerizer_tests.cpp:978: Should have a space between // and comment [whitespace/comments] [4]
> 3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp:90: At least a single space is required between code and comments [whitespace/comments] [2]
> 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp:127: Should have a space between // and comment [whitespace/comments] [4]
> Doesn't seem to give any false-positives. All the above errors do have a missing leading white-space.
>
> Tried a commit for the following patch:
> diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
> index eb47b53..e579d20 100644
> --- a/src/tests/slave_tests.cpp
> +++ b/src/tests/slave_tests.cpp
> @@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
>
> AWAIT_READY(launchTask);
>
> - // Verify label key and value in slave state.json.
> + //Verify label key and value in slave state.json.
> Future<process::http::Response> response =
> process::http::get(slave.get(), "state.json");
>
> And the git commit hook correctly failed it:
> $ git commit -m "test"
> Checking 1 files
> src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
> Total errors found: 1
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 41617: Added a new category called
whitespace/mesos-comments to capture missing, leading,
white-space in comments
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41617/
-----------------------------------------------------------
(Updated Jan. 14, 2016, 1:59 a.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4231
https://issues.apache.org/jira/browse/MESOS-4231
Repository: mesos
Description
-------
Added a new category called whitespace/mesos-comments to capture missing, leading, white-space in comments
Diffs (updated)
-----
support/cpplint.py 6890e27f92603b025e25e4db01decf351c33c9a1
support/mesos-style.py 5ee17593153df7815426918a1da6e25cb1188bca
Diff: https://reviews.apache.org/r/41617/diff/
Testing (updated)
-------
Ran the modified mesos-style.py on the existing code-base and got the following errors:
src/authentication/cram_md5/authenticatee.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
src/authentication/cram_md5/authenticator.hpp:60: Should have a space between // and comment [whitespace/comments] [4]
src/authorizer/local/authorizer.hpp:84: Should have a space between // and comment [whitespace/comments] [4]
src/tests/containerizer/docker_containerizer_tests.cpp:978: Should have a space between // and comment [whitespace/comments] [4]
3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp:90: At least a single space is required between code and comments [whitespace/comments] [2]
3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp:127: Should have a space between // and comment [whitespace/comments] [4]
Doesn't seem to give any false-positives. All the above errors do have a missing leading white-space.
Tried a commit for the following patch:
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index eb47b53..e579d20 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -2228,7 +2228,7 @@ TEST_F(SlaveTest, DiscoveryInfoAndPorts)
AWAIT_READY(launchTask);
- // Verify label key and value in slave state.json.
+ //Verify label key and value in slave state.json.
Future<process::http::Response> response =
process::http::get(slave.get(), "state.json");
And the git commit hook correctly failed it:
$ git commit -m "test"
Checking 1 files
src/tests/slave_tests.cpp:2231: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 1
Thanks,
Avinash sridharan