mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 62214: Added JavaScript linter.
Date Mon, 11 Sep 2017 19:18:34 GMT

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



Can you add Kevin Klues to the review and ask him to take a look at the virtual env bootstrapping?
I'm assuming you based this on his one for the CLI?


src/webui/master/static/.eslintrc.js
Lines 6 (patched)
<https://reviews.apache.org/r/62214/#comment261338>

    Does this run successfully on the webui? Can you document for posterity how this config
was produced?



src/webui/master/static/.gitignore
Lines 1 (patched)
<https://reviews.apache.org/r/62214/#comment261336>

    Ditto here re: serving the file over HTTP.



src/webui/master/static/bootstrap
Lines 3-4 (patched)
<https://reviews.apache.org/r/62214/#comment261335>

    I think the master serves all assets under static:
    
    https://github.com/apache/mesos/blob/297b7042a7ef65aafca40832b5f8736e27e26ed2/src/master/master.cpp#L1122
    
    Which means that if this file is here it's served by the master.



support/mesos-style.py
Lines 298-302 (patched)
<https://reviews.apache.org/r/62214/#comment261331>

    Stale copy paste



support/mesos-style.py
Lines 307-311 (patched)
<https://reviews.apache.org/r/62214/#comment261334>

    Similarly to my comment below, if we had some virtual env abstraction this could be running
something within in:
    
    ```
    virtualenv.run_within(['eslint', ...]);
    ```



support/mesos-style.py
Lines 309 (patched)
<https://reviews.apache.org/r/62214/#comment261339>

    Is this applying a lint on the whole file or just the diff? Is it possible to do just
the diff with eslint?
    
    If not, we'll need to make sure the lint result is clean before we commit this, right?



support/mesos-style.py
Lines 332-334 (patched)
<https://reviews.apache.org/r/62214/#comment261332>

    Copy paste?



support/mesos-style.py
Lines 344-361 (patched)
<https://reviews.apache.org/r/62214/#comment261333>

    Can you document that we build the virtualenv by running bootstrap?
    
    It will be unfortunate to have the code here and in PyLinter diverge, have you considered
adding some kind of VirtualEnv class or making these standalone virtualenv functions here
for them to both reuse?



support/mesos-style.py
Lines 343-345 (original)
<https://reviews.apache.org/r/62214/#comment261330>

    What happened here? Should this be a different patch?


- Benjamin Mahler


On Sept. 11, 2017, 9:49 a.m., Armand Grillet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62214/
> -----------------------------------------------------------
> 
> (Updated Sept. 11, 2017, 9:49 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7924
>     https://issues.apache.org/jira/browse/MESOS-7924
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This linter runs when changes on a JavaScript file are being committed.
> The linter used is ESLint with a configuration based on our current JS
> code base. The linter and its dependencies (i.e. Node.js) are installed
> in a virtual environment using Virtualenv and then Nodeenv.
> 
> 
> Diffs
> -----
> 
>   src/.gitignore PRE-CREATION 
>   src/webui/master/static/.eslintrc.js PRE-CREATION 
>   src/webui/master/static/.gitignore PRE-CREATION 
>   src/webui/master/static/bootstrap PRE-CREATION 
>   src/webui/master/static/pip-requirements.txt PRE-CREATION 
>   support/mesos-style.py cf37d9f4da4ab90b92f0136a1dcd5dd8bbae5785 
> 
> 
> Diff: https://reviews.apache.org/r/62214/diff/1/
> 
> 
> Testing
> -------
> 
> Following this commit, I have tried to commit a change on a JavaScript file and checked
that ESLinter was correctly invoked.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>


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