mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67055: Added Python 3 version of cpplint.
Date Mon, 14 May 2018 08:53:06 GMT

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



Since the diff contains a lot of new noise, at least I don't understand why this is needed.
There should be something in the commit message explaining it.

As always, I'd prefer to not fork this script absent a very strong reason. I believe the upstream
script should work fine with python2 or python3, let's make any new changes to a shared script
if possible.

- Benjamin Bannier


On May 11, 2018, 11:49 p.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67055/
> -----------------------------------------------------------
> 
> (Updated May 11, 2018, 11:49 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, Eric Chung, and Kevin
Klues.
> 
> 
> Bugs: MESOS-8770
>     https://issues.apache.org/jira/browse/MESOS-8770
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The Python 3 supports scripts currently live in support/python3.
> Once all support scripts will have been updated to Python 3 and
> tested by the developers, we will replace the Python 2 scripts by
> the new ones in an ongoing effort of modernizing our Python codebase.
> 
> 
> Diffs
> -----
> 
>   support/python3/cpplint.patch PRE-CREATION 
>   support/python3/cpplint.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67055/diff/2/
> 
> 
> Testing
> -------
> 
> Used `mesos-style.py` with the old `cpplint.py` (no change on `mesos-style.py`):
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
>   py27-lint: commands succeeded
>   congratulations :)
>   py27-lint: commands succeeded
>   congratulations :)
> Total errors found: 0
> ```
> Updated `mesos-style.py` to use the new version of cpplint: 
> ```
>         # We do not use a version of cpplint available through pip as
>         # we use a custom version (see cpplint.path) to lint C++ files.
>         process = subprocess.Popen(
>             ['python3', 'support/python3/cpplint.py', rules_filter] + source_paths,
>             stderr=subprocess.PIPE,
>             close_fds=True)
> ```
> 
> Run `mesos-style.py` again:
> ```
> apache-mesos (cpplint) $ ./support/mesos-style.py
> Checking 1289 C++ files
> Total errors found: 0
> Checking 4 JavaScript files
> Total errors found: 0
> Checking 47 Python files
>   py27-lint: commands succeeded
>   congratulations :)
>   py27-lint: commands succeeded
>   congratulations :)
> ************* Module mesos-style
> C:279, 0: Line too long (83/80) (line-too-long)
> Total errors found: 1
> ```
> As we can see, the number of errors found by cpplint is the same as before.
> 
> I have also used cpplint separately bu running `python3 support/python3/cpplint` on a
few C++ files to see that it was running as expected.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


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