Re: Review Request 62333: Added class for linters using a virtual environment.

[Armand Grillet]

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

(Updated Sept. 25, 2017, 2:55 p.m.)


Review request for mesos, Benjamin Mahler and Kevin Klues.


Changes
---

Resolved issues.


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


Repository: mesos


Description (updated)
---

This is used by the Python linter and
will be used by the Javascript linter.


Diffs (updated)
-

  support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 


Diff: https://reviews.apache.org/r/62333/diff/2/

Changes: https://reviews.apache.org/r/62333/diff/1-2/


Testing
---

Manual updates of `.py` and `.js` files then test commits to see if the linters 
before and afer removing their `.virtualenv` were still working as expected.


Thanks,

Armand Grillet

Re: Review Request 62333: Added class for linters using a virtual environment.

[Armand Grillet]

> On Sept. 19, 2017, 10:35 p.m., Benjamin Mahler wrote:
> > support/mesos-style.py
> > Lines 253-254 (patched)
> > 
> >
> > Is this clear to you? I don't understand why there is this distinction. 
> > My intuition would tell me that we don't need to rebuild the virtualenv if 
> > it's already built, but of course this code seems to suggest we always 
> > rebuild for a full lint?

After checking the history of the file, the logic behind `file_list` can be 
found in this commit: 
https://github.com/apache/mesos/commit/2e55afb35a912d0b6f8968bfb34832e645cb7903.
 Commit message:
```
The python linter needs to be run in a virtualenv due to how
it requires multiple modules that may or may not be installed
(or the correct version) in the host environment.  We originally
re-made this virtualenv whenever some specific files were
changed (such as the pip-requirements.txt, which describes
the virtualenv's dependencies).

However, this logic was missing the case where we ran
support/mesos-style.py with no arguments.  In this case, the
entire codebase is linted.  Therefore, the virtualenv should
also be rebuilt in this case.
```


- Armand


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


On Sept. 25, 2017, 2:55 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62333/
> ---
> 
> (Updated Sept. 25, 2017, 2:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kevin Klues.
> 
> 
> Bugs: MESOS-7924
> https://issues.apache.org/jira/browse/MESOS-7924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used by the Python linter and
> will be used by the Javascript linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 9cd1623c48623e558dcd5b80b4bbf5a2162c57cb 
> 
> 
> Diff: https://reviews.apache.org/r/62333/diff/2/
> 
> 
> Testing
> ---
> 
> Manual updates of `.py` and `.js` files then test commits to see if the 
> linters before and afer removing their `.virtualenv` were still working as 
> expected.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62333: Added class for linters using a virtual environment.

[Benjamin Mahler]

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




support/mesos-style.py
Lines 209 (patched)


How about VirtualenvLinterBase?

But, do you need an abstract class for this? Why not just some functions?



support/mesos-style.py
Lines 214 (patched)


Can you document what this returns? Or that rather than returning anything 
it handles failure by exiting the program?



support/mesos-style.py
Lines 233 (patched)


I'm left confused as a reader as to why this one takes a file list but 
build_virtualenv does not take file list, what is the file list?

Can you document this?



support/mesos-style.py
Lines 253-254 (patched)


Is this clear to you? I don't understand why there is this distinction. My 
intuition would tell me that we don't need to rebuild the virtualenv if it's 
already built, but of course this code seems to suggest we always rebuild for a 
full lint?


- Benjamin Mahler


On Sept. 14, 2017, 3:34 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62333/
> ---
> 
> (Updated Sept. 14, 2017, 3:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Kevin Klues.
> 
> 
> Bugs: MESOS-7924
> https://issues.apache.org/jira/browse/MESOS-7924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is used by the Python linter and
> will be used by the JavaScript linter.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 
> 
> 
> Diff: https://reviews.apache.org/r/62333/diff/1/
> 
> 
> Testing
> ---
> 
> Manual updates of `.py` and `.js` files then test commits to see if the 
> linters before and afer removing their `.virtualenv` were still working as 
> expected.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 62333: Added class for linters using a virtual environment.

[Armand Grillet]

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

Review request for mesos, Benjamin Mahler and Kevin Klues.


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


Repository: mesos


Description
---

This is used by the Python linter and
will be used by the JavaScript linter.


Diffs
-

  support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 


Diff: https://reviews.apache.org/r/62333/diff/1/


Testing
---

Manual updates of `.py` and `.js` files then test commits to see if the linters 
before and afer removing their `.virtualenv` were still working as expected.


Thanks,

Armand Grillet