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


Fix it, then Ship it!




This is looking good! Some nits and we're good to go.


src/common/command_utils.hpp (line 48)
<https://reviews.apache.org/r/42662/#comment177650>

    Why untar has a default for 'directory' while tar does not?



src/common/command_utils.hpp (line 56)
<https://reviews.apache.org/r/42662/#comment177644>

    Please update the comments accordingly.



src/common/command_utils.hpp (line 59)
<https://reviews.apache.org/r/42662/#comment177643>

    s/inputFilePath/input/



src/common/command_utils.cpp (line 19)
<https://reviews.apache.org/r/42662/#comment177646>

    is it used? Please make sure to remove headers that are not used.



src/common/command_utils.cpp (line 31)
<https://reviews.apache.org/r/42662/#comment177645>

    is it used?



src/common/command_utils.cpp (line 45)
<https://reviews.apache.org/r/42662/#comment177649>

    s/command/path/ to be consistent with subprocess.



src/common/command_utils.cpp (lines 55 - 56)
<https://reviews.apache.org/r/42662/#comment177648>

    Instead of saving a lambda, can we just save the string that we want to log?
    
    ```
    string command = strings::join(
        ", ",
        path,
        strings::join(", ", argv));
    ```



src/common/command_utils.cpp (lines 63 - 73)
<https://reviews.apache.org/r/42662/#comment177647>

    You should be able to use strings::join here.



src/common/command_utils.cpp (line 143)
<https://reviews.apache.org/r/42662/#comment177651>

    I don't think we need to check if 'directory' is empty or not. Otherwise, 
do you need to check if 'input' or 'output' is empty or not?



src/common/command_utils.cpp (line 161)
<https://reviews.apache.org/r/42662/#comment177652>

    Please align '{' here accordingly.



src/common/command_utils.cpp (line 183)
<https://reviews.apache.org/r/42662/#comment177653>

    Ditto on empty check.



src/tests/common/command_utils_tests.cpp (line 23)
<https://reviews.apache.org/r/42662/#comment177654>

    Why do you need this? Please remove headers that are not needed please.



src/tests/common/command_utils_tests.cpp (line 33)
<https://reviews.apache.org/r/42662/#comment177655>

    we try to avoid using 'using namespace' clause. For this test, I think it's 
pretty clean and readable to use command::tar, command::untar (i.e., no need 
for this clause).



src/tests/common/command_utils_tests.cpp (line 43)
<https://reviews.apache.org/r/42662/#comment177657>

    I would suggest to rename it to TarTest. Two reasons: 1) we might have 'ar' 
util in the future 2) it's better to keep the name consistent with the command 
name to readability and searchability.



src/tests/common/command_utils_tests.cpp (line 46)
<https://reviews.apache.org/r/42662/#comment177659>

    s/createTestFileInDir/createTestFile/
    
    InDir is implied by the parameter 'directory'



src/tests/common/command_utils_tests.cpp (line 47)
<https://reviews.apache.org/r/42662/#comment177658>

    s/filePath/file/
    
    Path is the type, no need for the redundent Path in variable name.



src/tests/common/command_utils_tests.cpp (line 50)
<https://reviews.apache.org/r/42662/#comment177660>

    s/testFilePath/testFile/



src/tests/common/command_utils_tests.cpp (line 54)
<https://reviews.apache.org/r/42662/#comment177661>

    Do you need this temp variable?



src/tests/common/command_utils_tests.cpp (lines 93 - 95)
<https://reviews.apache.org/r/42662/#comment177662>

    You can use EXPECT_SOME_EQ("test", os::read(testFile));



src/tests/common/command_utils_tests.cpp (line 103)
<https://reviews.apache.org/r/42662/#comment177663>

    s/testFilePath/testFile/



src/tests/common/command_utils_tests.cpp (lines 122 - 124)
<https://reviews.apache.org/r/42662/#comment177664>

    Ditto on using EXPECT_SOME_EQ



src/tests/common/command_utils_tests.cpp (line 135)
<https://reviews.apache.org/r/42662/#comment177665>

    s/testFilePath/testFile/



src/tests/common/command_utils_tests.cpp (lines 157 - 159)
<https://reviews.apache.org/r/42662/#comment177667>

    Ditto on using EXPECT_SOME_EQ



src/tests/common/command_utils_tests.cpp (lines 190 - 192)
<https://reviews.apache.org/r/42662/#comment177668>

    Ditto.



src/tests/common/command_utils_tests.cpp (line 205)
<https://reviews.apache.org/r/42662/#comment177669>

    s/testFilePath/testFile/



src/tests/common/command_utils_tests.cpp (lines 212 - 213)
<https://reviews.apache.org/r/42662/#comment177670>

    Can you do the indentation like the following so that it's consistent with 
your style in BZIP2CompressFile
    ```
    AWAIT_ASSERT_READY(tar(
        testDir,
        outputTarFile,
        topDir,
        Compression::GZIP));
    ```



src/tests/common/command_utils_tests.cpp (lines 229 - 231)
<https://reviews.apache.org/r/42662/#comment177671>

    Ditto on using EXPECT_SOME_EQ


- Jie Yu


On Jan. 26, 2016, 2:18 a.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> -----------------------------------------------------------
> 
> (Updated Jan. 26, 2016, 2:18 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -----
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Reply via email to