Re: Review Request 23710: Add line comments end punctuation style rule

[Niklas Nielsen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review55831
---


Hey Tim, did you get to address Adam's comment on the off-by-one issue?

- Niklas Nielsen


On Aug. 25, 2014, 5:12 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 25, 2014, 5:12 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

I see, it collapsed multi line comments into a single line so line numbers are 
off..

I'll figure something out soon.

Tim

Sent from my iPhone

> On Aug 26, 2014, at 7:30 PM, "Adam B"  wrote:
> 
> 
> This is an automatically generated e-mail. To reply, visit: 
> https://reviews.apache.org/r/23710/
> 
> On August 25th, 2014, 1:01 a.m. PDT, Adam B wrote:
> 
> support/cpplint.py (Diff revision 2)
> 1550  
>   error(filename, i, 'readability/ending_punctuation', 3,
> 1551  
> 'Comment should end in non-alphabetical character. \n Line: ' 
> +
> 1552  
> previous_line + '\n')
> s/i/i-1/?
> On August 25th, 2014, 4:03 p.m. PDT, Timothy Chen wrote:
> 
> ah, I think it should be i+1? since it's 0 based index.
> On August 25th, 2014, 4:51 p.m. PDT, Timothy Chen wrote:
> 
> Actually it should be i because of the padding done.
> On August 25th, 2014, 11:54 p.m. PDT, Adam B wrote:
> 
> What testing was done on this code? 'make check' hardly seems relevant.
> 
> Did you verify that these style errors print the correct line number?
> I just tested this with a malformed comment on line 1 and line 4079 (last) of 
> a file. Here's what it prints:
> 
> src/slave/slave.cpp:19:  Comment should end in non-alphabetical character.
> 
>  Line: // foo bar
> 
> src/slave/slave.cpp:4080:  Comment should end in non-alphabetical character.
> 
>  Line: // foo bar
> 
> Looks like the line number calculation is wrong. :(
> 
> - Adam
> 
> 
> On August 25th, 2014, 5:12 p.m. PDT, Timothy Chen wrote:
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> By Timothy Chen.
> Updated Aug. 25, 2014, 5:12 p.m.
> 
> Repository: mesos-git
> Description
> 
> Review: https://reviews.apache.org/r/23710
> Testing
> 
> make check
> Diffs
> 
> support/cpplint.patch (1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0)
> support/cpplint.py (bfd3390002a680b07aa3fcf785279ad19625294b)
> support/mesos-style.py (d24cb11adc06bc0ebaaa206301616c8b597f09e8)
> View Diff

Re: Review Request 23710: Add line comments end punctuation style rule

[Adam B]

> On Aug. 25, 2014, 1:01 a.m., Adam B wrote:
> > support/cpplint.py, lines 1550-1552
> > 
> >
> > s/i/i-1/?
> 
> Timothy Chen wrote:
> ah, I think it should be i+1? since it's 0 based index.
> 
> Timothy Chen wrote:
> Actually it should be i because of the padding done.
> 
> Adam B wrote:
> What testing was done on this code? 'make check' hardly seems relevant.
> Did you verify that these style errors print the correct line number?

I just tested this with a malformed comment on line 1 and line 4079 (last) of a 
file. Here's what it prints:
src/slave/slave.cpp:19:  Comment should end in non-alphabetical character.
 Line: // foo bar
src/slave/slave.cpp:4080:  Comment should end in non-alphabetical character.
 Line: // foo bar
Looks like the line number calculation is wrong. :(


- Adam


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


On Aug. 25, 2014, 5:12 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 25, 2014, 5:12 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Adam B]

> On Aug. 25, 2014, 1:01 a.m., Adam B wrote:
> > support/cpplint.py, line 1540
> > 
> >
> > Why 'continue' on first and last line? If you skip this early, you'll 
> > never set "previous_line = line" to catch punctuation errors in comments on 
> > these lines?
> 
> Timothy Chen wrote:
> I need to skip the first and last line because ProcessFileData in 
> cpplint.py adds a extra line in the beginning and in the end for line numbers.

Fair enough. Can you put in a comment explaining this and the dummy comment?


> On Aug. 25, 2014, 1:01 a.m., Adam B wrote:
> > support/cpplint.py, lines 1550-1552
> > 
> >
> > s/i/i-1/?
> 
> Timothy Chen wrote:
> ah, I think it should be i+1? since it's 0 based index.
> 
> Timothy Chen wrote:
> Actually it should be i because of the padding done.

What testing was done on this code? 'make check' hardly seems relevant.
Did you verify that these style errors print the correct line number?


- Adam


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


On Aug. 25, 2014, 5:12 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 25, 2014, 5:12 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/
---

(Updated Aug. 26, 2014, 12:12 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.


Repository: mesos-git


Description
---

Review: https://reviews.apache.org/r/23710


Diffs (updated)
-

  support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
  support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
  support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 

Diff: https://reviews.apache.org/r/23710/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

> On Aug. 25, 2014, 8:01 a.m., Adam B wrote:
> > support/cpplint.py, lines 1550-1552
> > 
> >
> > s/i/i-1/?
> 
> Timothy Chen wrote:
> ah, I think it should be i+1? since it's 0 based index.

Actually it should be i because of the padding done.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


On Aug. 20, 2014, 12:36 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 20, 2014, 12:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

> On Aug. 25, 2014, 8:01 a.m., Adam B wrote:
> > support/cpplint.py, line 1540
> > 
> >
> > Why 'continue' on first and last line? If you skip this early, you'll 
> > never set "previous_line = line" to catch punctuation errors in comments on 
> > these lines?

I need to skip the first and last line because ProcessFileData in cpplint.py 
adds a extra line in the beginning and in the end for line numbers.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


On Aug. 20, 2014, 12:36 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 20, 2014, 12:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

> On Aug. 25, 2014, 8:01 a.m., Adam B wrote:
> > support/cpplint.py, lines 1550-1552
> > 
> >
> > s/i/i-1/?

ah, I think it should be i+1? since it's 0 based index.


- Timothy


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


On Aug. 20, 2014, 12:36 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 20, 2014, 12:36 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Adam B]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review51382
---


Looks mostly good, but I'm a little worried about the first & last lines of 
files. Doubtful that they would have useful comments, but I'd like us to check 
them for principle's sake.


support/cpplint.py


url, path, or parameter.



support/cpplint.py


Why 'continue' on first and last line? If you skip this early, you'll never 
set "previous_line = line" to catch punctuation errors in comments on these 
lines?



support/cpplint.py


s/i/i-1/?



support/cpplint.py


". \n"  should be ".\n" (same for line 1559)



support/mesos-style.py


Can you alphabetize these while you're in there?


- Adam B


On Aug. 19, 2014, 5:36 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated Aug. 19, 2014, 5:36 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/23710
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
>   support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
>   support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/
---

(Updated Aug. 20, 2014, 12:36 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.


Repository: mesos-git


Description (updated)
---

Review: https://reviews.apache.org/r/23710


Diffs (updated)
-

  support/cpplint.patch 1dd69a03044c0c17ca4ee555c6c9d27ea043d4f0 
  support/cpplint.py bfd3390002a680b07aa3fcf785279ad19625294b 
  support/mesos-style.py d24cb11adc06bc0ebaaa206301616c8b597f09e8 

Diff: https://reviews.apache.org/r/23710/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/
---

(Updated Aug. 20, 2014, 12:32 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.


Repository: mesos-git


Description (updated)
---

Add line comments end punctuation style rule.

This rule checks the last line of the // comment lines to make sure the last 
word is ended with a non alphabetical character.
It allows some exceptions for the last word, such as the word contains a url 
(://), starts with path (/) or a command (--).


Diffs
-

  support/cpplint.patch 4f1ec668822b3ea3d8bfb12d7d41408ccaa42271 
  support/cpplint.py 90aa4ba648ce83f84ec62d96e26eba5e7f11f30c 
  support/mesos-style.py fd12be9ad1647e658bca45f181f9aa9da1a15084 

Diff: https://reviews.apache.org/r/23710/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 23710: Add line comments end punctuation style rule

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review49349
---



support/cpplint.py


I believe that's with command line parameters yes. I can add a comment.



support/cpplint.py


cpplint.py replaces multi-line comments with // dummy. So skipping those 
replaced comments altogether as the replaced comment violates this rule


- Timothy Chen


On July 19, 2014, 8:53 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated July 19, 2014, 8:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add line comments end punctuation style rule
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 4f1ec668822b3ea3d8bfb12d7d41408ccaa42271 
>   support/cpplint.py 90aa4ba648ce83f84ec62d96e26eba5e7f11f30c 
>   support/mesos-style.py fd12be9ad1647e658bca45f181f9aa9da1a15084 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Adam B]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review49333
---



support/cpplint.patch


Please also add a "Modified by" line(s) explaining the new rule you added.



support/cpplint.py


What is the startswith("--") for? Command-line parameters? It's not 
mentioned in your comment above.



support/cpplint.py


What's with the "// dummy" exemption?



support/cpplint.py


Do you really think this is severity '5'? I'd probably drop it down to 3 at 
most.



support/cpplint.py


"Could not allow..." reads funny. How about just "Comment should end in 
punctuation.", or "Comment should end in non-alphabetical character." if you 
want to be more exact?



support/cpplint.py


Reads like broken English, and is too specific about the 'Period'. How 
about "CheckForEndingPunctuationInComments" or 
"CheckForCommentsEndingInPunctuation"?



support/mesos-style.py


You could probably change this to "readability/ending_punctuation" to keep 
it down to two words. The fact that it applies to comments is secondary; that's 
just where you're applying the ending_punctuation rule.


- Adam B


On July 19, 2014, 1:53 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated July 19, 2014, 1:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add line comments end punctuation style rule
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 4f1ec668822b3ea3d8bfb12d7d41408ccaa42271 
>   support/cpplint.py 90aa4ba648ce83f84ec62d96e26eba5e7f11f30c 
>   support/mesos-style.py fd12be9ad1647e658bca45f181f9aa9da1a15084 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Niklas Nielsen]

> On July 22, 2014, 12:25 a.m., Mesos ReviewBot wrote:
> > Bad patch!
> > 
> > Reviews applied: [23596, 23597, 23598, 23599, 23707]
> > 
> > Failed command: git apply --index 23707.patch
> > 
> > Error:
> >  error: patch failed: 
> > src/slave/containerizer/isolators/network/port_mapping.cpp:491
> > error: src/slave/containerizer/isolators/network/port_mapping.cpp: patch 
> > does not apply
> >

Want to rebase? :-)


- Niklas


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review48349
---


On July 19, 2014, 1:53 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated July 19, 2014, 1:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add line comments end punctuation style rule
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 4f1ec668822b3ea3d8bfb12d7d41408ccaa42271 
>   support/cpplint.py 90aa4ba648ce83f84ec62d96e26eba5e7f11f30c 
>   support/mesos-style.py fd12be9ad1647e658bca45f181f9aa9da1a15084 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 23710: Add line comments end punctuation style rule

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23710/#review48349
---


Bad patch!

Reviews applied: [23596, 23597, 23598, 23599, 23707]

Failed command: git apply --index 23707.patch

Error:
 error: patch failed: 
src/slave/containerizer/isolators/network/port_mapping.cpp:491
error: src/slave/containerizer/isolators/network/port_mapping.cpp: patch does 
not apply


- Mesos ReviewBot


On July 19, 2014, 8:53 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23710/
> ---
> 
> (Updated July 19, 2014, 8:53 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Niklas Nielsen.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> ---
> 
> Add line comments end punctuation style rule
> 
> 
> Diffs
> -
> 
>   support/cpplint.patch 4f1ec668822b3ea3d8bfb12d7d41408ccaa42271 
>   support/cpplint.py 90aa4ba648ce83f84ec62d96e26eba5e7f11f30c 
>   support/mesos-style.py fd12be9ad1647e658bca45f181f9aa9da1a15084 
> 
> Diff: https://reviews.apache.org/r/23710/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>