mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ben Mahler <benjamin.mah...@gmail.com>
Subject Re: Review Request 43220: Added script to generate docs from endpoint help strings.
Date Sat, 06 Feb 2016 01:46:39 GMT

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



Looking great, just some minor thoughts around ways to make this a bit easier on the reader
of the code. Curious to hear your thoughts!


support/generate-endpoint-help.py (line 35)
<https://reviews.apache.org/r/43220/#comment179350>

    typo: cause



support/generate-endpoint-help.py (line 36)
<https://reviews.apache.org/r/43220/#comment179351>

    typo: we



support/generate-endpoint-help.py (lines 55 - 60)
<https://reviews.apache.org/r/43220/#comment179358>

    Do we need the name indexing? It looks like we could just store the Popen objects in a
list for cleanup purposes. Could start_master just return the Popen object instead of the
index into this map?
    
    Since we only create the master followed by the agent, we don't have both subprocesses
alive at the same time, could we simplify if we assume we won't have more than a single active
subprocess for now?



support/generate-endpoint-help.py (lines 76 - 92)
<https://reviews.apache.org/r/43220/#comment179352>

    Would be nice to indent consistently here if possible, should we wrap the add_argument
calls on a newline?



support/generate-endpoint-help.py (lines 77 - 79)
<https://reviews.apache.org/r/43220/#comment179353>

    Should this be a 4 space indent?



support/generate-endpoint-help.py (lines 100 - 111)
<https://reviews.apache.org/r/43220/#comment179355>

    Do we need these? They seem to exist only to rename os.makedirs and shutil.rmtree?
    
    For the os.makedirs error handling, we could just do the following in the caller code
to make the 'already exists' case explicit:
    
    ```
    if not os.path.exists(directory):
        os.makedirs(directory)
    ```



support/generate-endpoint-help.py (line 103)
<https://reviews.apache.org/r/43220/#comment179354>

    Should this be a 2 space indent?



support/generate-endpoint-help.py (lines 114 - 127)
<https://reviews.apache.org/r/43220/#comment179356>

    Could we avoid the string -> list conversion by just starting with the command in list
form?
    
    It looks like the caller code doesn't need to use a string.
    
    After that change, this could take s/cmd/args/



support/generate-endpoint-help.py (lines 133 - 134)
<https://reviews.apache.org/r/43220/#comment179357>

    Where does this happen? Looks like we don't delete the key from the map after the kill?



support/generate-endpoint-help.py (lines 186 - 188)
<https://reviews.apache.org/r/43220/#comment179359>

    A maintenance endpoint might be a good example to include in general, since these endpoints
have a slash in the name?



support/generate-endpoint-help.py (lines 242 - 243)
<https://reviews.apache.org/r/43220/#comment179360>

    Could we prefix this sentence with a bold NOTE?



support/generate-endpoint-help.py (line 255)
<https://reviews.apache.org/r/43220/#comment179361>

    Let's omit any reference to mesosphere ;)



support/generate-endpoint-help.py (line 264)
<https://reviews.apache.org/r/43220/#comment179362>

    Ditto here ;)



support/generate-endpoint-help.py (lines 270 - 280)
<https://reviews.apache.org/r/43220/#comment179363>

    Still a little tricky for me to figure this out, could the comment include an example?
    
    One thought that comes to mind is, should this just return a 'list' of links for a given
process, and the caller loops over processes calling this? Then the caller loops over the
link list and stringifies as needed. Just thinking that perhaps this function should not be
doing the overall string construction for the caller as that might be a bit tougher to follow.
    
    Curious to see if we can make this easier for dummies like me :)



support/generate-endpoint-help.py (line 304)
<https://reviews.apache.org/r/43220/#comment179364>

    relative_path


- Ben Mahler


On Feb. 5, 2016, 11:17 p.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43220/
> -----------------------------------------------------------
> 
> (Updated Feb. 5, 2016, 11:17 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.
> 
> 
> Bugs: MESOS-3831
>     https://issues.apache.org/jira/browse/MESOS-3831
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Invoke this script from anywhere within the mesos source tree
> (including in the build directory) to autogenerate documentation for
> all endpoint strings and install them into the docs/endpoint folder.
> In a subsequent commit we commit all of these autogenerated files back
> to the source repo and add a link in home.md to find them.
> 
> 
> Diffs
> -----
> 
>   support/generate-endpoint-help.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43220/diff/
> 
> 
> Testing
> -------
> 
> Ran and generated files from it.  Then rebuilt the website and verified that everything
looked as it should.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>


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