You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@mesos.apache.org by Steven Phung <st...@gmail.com> on 2014/05/13 05:25:21 UTC

Review Request 21360: Optimized mesos-style.py to run cpplint on staged files only

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

Review request for mesos and Vinod Kone.


Bugs: MESOS-1349
    https://issues.apache.org/jira/browse/MESOS-1349


Repository: mesos-git


Description
-------

Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
    
git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.


Diffs
-----

  support/mesos-style.py bd7dcdb 

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


Testing
-------

./support/mesos-style.py


Thanks,

Steven Phung

Re: Review Request 21360: Optimized mesos-style.py to run cpplint on staged files only

Posted by Vinod Kone <vi...@gmail.com>.

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



support/mesos-style.py
<https://reviews.apache.org/r/21360/#comment76735>

    How about taking the list of file names to run the linter against as command line args to mesos-style.py?. I think thats more flexible and lets users feed it whatever files they want.
    
    e.g., 'git diff --name-only | xargs ./support/mesos-style.py'


- Vinod Kone


On May 13, 2014, 3:25 a.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 3:25 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Steven Phung <st...@gmail.com>.


> On May 13, 2014, 10:25 p.m., Vinod Kone wrote:
> > Sorry for not being clear. I think the script should by default look at the whole source tree. Only when a list of files is given as cmd args should it restrict it to those files (with filtering). Much like how git commands work (e.g., 'git diff' applies to whole repo, whereas 'git diff -- src/' applies to only source directory).
> > 
> > I agree that it might be weird if someone specifies a file in cmd arg and we filter it based on our filtering rules (3rd party, generated etc) but I think that's fine for now and should work with our current use cases (review bot, users doing it manually, git commit hook). We could, in the future, add an option (--force or something) that could override the default filtering rules too if someone needs it.

That makes sense, I'll update the diff sometime tonight.  Thanks for the clarification.


- Steven


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


On May 13, 2014, 9:56 p.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 9:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Vinod Kone <vi...@gmail.com>.

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

Sorry for not being clear. I think the script should by default look at the whole source tree. Only when a list of files is given as cmd args should it restrict it to those files (with filtering). Much like how git commands work (e.g., 'git diff' applies to whole repo, whereas 'git diff -- src/' applies to only source directory).

I agree that it might be weird if someone specifies a file in cmd arg and we filter it based on our filtering rules (3rd party, generated etc) but I think that's fine for now and should work with our current use cases (review bot, users doing it manually, git commit hook). We could, in the future, add an option (--force or something) that could override the default filtering rules too if someone needs it.

- Vinod Kone

On May 13, 2014, 9:56 p.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 9:56 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Vinod Kone <vi...@gmail.com>.


> On May 14, 2014, 6:07 a.m., Vinod Kone wrote:
> > Ship It!

Committed this with a minor fix. Thanks Steven.


- Vinod


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


On May 14, 2014, 4:24 a.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 4:24 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Vinod Kone <vi...@gmail.com>.

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

Ship it!


Ship It!

- Vinod Kone


On May 14, 2014, 4:24 a.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 4:24 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Vinod Kone <vi...@gmail.com>.


> On May 14, 2014, 5:12 a.m., Dominic Hamon wrote:
> > support/mesos-style.py, line 93
> > <https://reviews.apache.org/r/21360/diff/3/?file=581314#file581314line93>
> >
> >     could we just run find_candidates on each of the file_paths in this case and avoid the cost of generating the candidates set in the first place?

While this is a good idea, a quick test suggests that generating "candidates" list is very fast in our repo. I propose we do this optimization in a follow up review, if necessary. Sounds good?


- Vinod


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


On May 14, 2014, 4:24 a.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 4:24 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Dominic Hamon <dh...@twopensource.com>.

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



support/mesos-style.py
<https://reviews.apache.org/r/21360/#comment76891>

    could we just run find_candidates on each of the file_paths in this case and avoid the cost of generating the candidates set in the first place?


- Dominic Hamon


On May 13, 2014, 9:24 p.m., Steven Phung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21360/
> -----------------------------------------------------------
> 
> (Updated May 13, 2014, 9:24 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1349
>     https://issues.apache.org/jira/browse/MESOS-1349
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
>     
> git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py bd7dcdb 
> 
> Diff: https://reviews.apache.org/r/21360/diff/
> 
> 
> Testing
> -------
> 
> ./support/mesos-style.py
> 
> 
> Thanks,
> 
> Steven Phung
> 
>

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Steven Phung <st...@gmail.com>.

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

(Updated May 14, 2014, 4:24 a.m.)

Review request for mesos and Vinod Kone.

Changes
-------

Updated for to reflect Vinod's clarifications.

The default behavior when no args are provided is to check the entire source tree based on the defined rules in code.

If a list of arguments is provided they represent file paths to run lint on. The file paths that are passed in will be filtered down based on the defined rules in mesos-style.py which means if it is not a candidate for lint then it will be ignored. (For now)

Bugs: MESOS-1349
https://issues.apache.org/jira/browse/MESOS-1349

Repository: mesos-git

Description
-------

Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.

git diff is used to determine which staged files that have been added or removed. The intersection between the staged files and all lint candidates is computed and sent to cpplint. The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.

Diffs (updated)
-----

support/mesos-style.py bd7dcdb

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

Testing
-------

./support/mesos-style.py

Thanks,

Steven Phung

Re: Review Request 21360: mesos-style.py accepts files to lint as args

Posted by Steven Phung <st...@gmail.com>.

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

(Updated May 13, 2014, 9:56 p.m.)


Review request for mesos and Vinod Kone.


Summary (updated)
-----------------

mesos-style.py accepts files to lint as args


Bugs: MESOS-1349
    https://issues.apache.org/jira/browse/MESOS-1349


Repository: mesos-git


Description
-------

Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.
    
git diff is used to determine which staged files that have been added or removed.  The intersection between the staged files and all lint candidates is computed and sent to cpplint.  The return value of the mesos-style.py script is the result of the cpplint process exit or 0 if there the intersection is empty.


Diffs
-----

  support/mesos-style.py bd7dcdb 

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


Testing
-------

./support/mesos-style.py


Thanks,

Steven Phung

Re: Review Request 21360: Optimized mesos-style.py to run cpplint on staged files only

Posted by Steven Phung <st...@gmail.com>.

(Updated May 13, 2014, 9:53 p.m.)

Review request for mesos and Vinod Kone.

Changes
-------

mesos-style.py accepts files to lint as args

Accepting the files to lint as args allows users to check the style of specific files. Using a command like git diff piped to xargs enables checking against files that are added or modified. The responsibility of filtering files was removed because it seems to make the most sense to have mesos-style run against what is given to it rather than filtering certain inputs based on the extension.

Bugs: MESOS-1349
https://issues.apache.org/jira/browse/MESOS-1349

Repository: mesos-git

Description
-------

Previously run_lint was being invoked a large subset of the source tree which was inefficient. With this change it enables mesos-style.py to potentially be used as a pre-commit hook.

Diffs (updated)
-----

support/mesos-style.py bd7dcdb

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

Testing
-------

./support/mesos-style.py

Thanks,

Steven Phung