mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 50910: Added a python linter to mesos-style.cpp.
Date Mon, 10 Oct 2016 17:59:24 GMT


> On Oct. 4, 2016, 5:18 p.m., Joseph Wu wrote:
> > support/mesos-style.py, lines 270-272
> > <https://reviews.apache.org/r/50910/diff/4/?file=1471508#file1471508line270>
> >
> >     It may help to change the order of reviews (no rebase necessary, just the "depends
on" field).  
> >     
> >     Because you add a linter in this review, that depends on the next one ( https://reviews.apache.org/r/50912/
), this review fails to lint itself :)
> 
> Kevin Klues wrote:
>     I'm confused. The order of these patches (and their dependencies) should be correct.
This patch "depends on" 50907, not 50912. Also, when I cherry-pick each of these patches onto
master individually, they all get linted just as expected.
> 
> Kevin Klues wrote:
>     For example, from master:
>     ```
>     klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50907
>     2016-10-04 20:31:30 URL:https://reviews.apache.org/r/50907/diff/raw/ [16055/16055]
-> "50907.patch" [1]
>     No C++ files to lint
>     [cli-test d536361] Abstracted mesos-style.py to wrap the cpp linter in a class.
>      1 file changed, 249 insertions(+), 169 deletions(-)
>      rewrite support/mesos-style.py (95%)
>     klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50910
>     2016-10-04 20:31:39 URL:https://reviews.apache.org/r/50910/diff/raw/ [2997/2997]
-> "50910.patch" [1]
>     No C++ files to lint
>     No Python files to lint
>     [cli-test 2b9502e] Added a python linter to mesos-style.cpp.
>      Author: Haris Choudhary <haris05ch@gmail.com>
>      1 file changed, 67 insertions(+), 2 deletions(-)
>     klueska@c99:~/projects/mesos$ support/apply-reviews.py -r 50912
>     2016-10-04 20:31:51 URL:https://reviews.apache.org/r/50912/diff/raw/ [40930/40930]
-> "50912.patch" [1]
>     No C++ files to lint
>     Checking 8 Python files
>     Total errors found: 0
>     [cli-test 88e5726] Added the infrastructure for a new python-based CLI.
>      20 files changed, 1019 insertions(+), 3 deletions(-)
>      create mode 100644 src/cli_new/.gitignore
>      create mode 100644 src/cli_new/README.md
>      create mode 100644 src/cli_new/activate
>      create mode 100644 src/cli_new/bin/config.py
>      create mode 100644 src/cli_new/bin/main.py
>      create mode 100755 src/cli_new/bin/mesos
>      create mode 100755 src/cli_new/bootstrap
>      create mode 100644 src/cli_new/deactivate
>      create mode 100644 src/cli_new/lib/mesos/__init__.py
>      create mode 100644 src/cli_new/lib/mesos/docopt.py
>      create mode 100644 src/cli_new/lib/mesos/exceptions.py
>      create mode 100644 src/cli_new/lib/mesos/plugins/__init__.py
>      create mode 100644 src/cli_new/lib/mesos/plugins/base.py
>      create mode 100644 src/cli_new/lib/mesos/util.py
>      create mode 100644 src/cli_new/mesos.bash_completion
>      create mode 100644 src/cli_new/pip-requirements.txt
>      create mode 100644 src/cli_new/pylint.config
>     ```
> 
> Joseph Wu wrote:
>     Presumably, that's because your source directory contains leftover directories/files
(like `*.pyc` files, which prevent git from deleting the `cli_new` folder):
>     ```
>     $ support/apply-reviews.py -n -r 50910 -c
>     2016-10-05 10:49:31 URL:https://reviews.apache.org/r/50907/diff/raw/ [16055/16055]
-> "50907.patch" [1]
>     No C++ files to lint
>     [detached HEAD faed65f] Abstracted mesos-style.py to wrap the cpp linter in a class.
>      Author: Kevin Klues <klueska@gmail.com>
>      1 file changed, 249 insertions(+), 169 deletions(-)
>      rewrite support/mesos-style.py (95%)
>     2016-10-05 10:49:32 URL:https://reviews.apache.org/r/50910/diff/raw/ [2997/2997]
-> "50910.patch" [1]
>     No C++ files to lint
>     Could not find 'src/cli_new'
>     Please run from the root of the mesos source directory
>     ```
>     
>     If I do:
>     ```
>     $ support/apply-reviews.py -n -r 50907
>     $ support/apply-reviews.py -n -r 50912 # <- reordered this one.
>     $ support/apply-reviews.py -n -r 50910
>     ```
>     
>     It applies cleanly.
> 
> Kevin Klues wrote:
>     I see. I think we should then add the linter in this commit (but with an emty array
of folders to check). And then introduce this folder into the array in the next commit.  Otherwise
we miss out on linting the entire base patch for the CLI.

Hm, probably not worth the trouble of splitting up the reviews.  I'll just do this:
```
$ support/apply-reviews.py -n -r 50907
$ mkdir src/cli_new
$ support/apply-reviews.py -n -r 50910  # Applies cleanly now.
```


- Joseph


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


On Aug. 11, 2016, 2:54 p.m., Haris Choudhary wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50910/
> -----------------------------------------------------------
> 
> (Updated Aug. 11, 2016, 2:54 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: Mesos-6006
>     https://issues.apache.org/jira/browse/Mesos-6006
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> It currently doesn't run over any files in the code base, but we will
> be adding the new python CLI in a subsequent commit, which will use
> this new linter.
> 
> 
> Diffs
> -----
> 
>   support/mesos-style.py 061cbe711bd9407b7341fa146f8c723eaa9fc438 
> 
> Diff: https://reviews.apache.org/r/50910/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message