This is an automatically generated e-mail. To reply, visit:

Thanks Yong for all your work on this!

Just a few comments about style and one comment regarding the use of 
`re.match()` instead of `re.search()`.

Please see my patch on github which applies all of the comments I've left:


support/mesos-style.py (line 34)

    We should probably call this `check_encoding()` instead of `run_ascii()` to 
be more consistent with `check_license_header()`.
    The `run_lint()` function actually calls out to a linter, so this name was 
more appropriate here.
    Also, `check_encoding()` is probably better than `check_ascii()` because it 
is a bit clearer as to our intent.

support/mesos-style.py (lines 35 - 37)

    We should probably have a bit longer description here, making it clear that 
this covers both traditional ascii characters (0-127) as well as the extended 
ascii characters (128-255). I would probably even link to one of the ascii 
tables online.

support/mesos-style.py (line 38)

    We typically put spaces between our variable names and our equal signs. 
    total_errors = 0
    Also, we should probably name this variable `error_count` instead of 
`total_errors` to be consistent with `check_license_header()`.
    In general, this function should probably follow the same structure as 
`check_license_header()` except for the regex.

support/mesos-style.py (lines 39 - 40)

    We should probably use the same variable names as `check_license_header()` 
for consistency here:
    for path in source_paths:
        with open(path) as source_file:

support/mesos-style.py (line 41)

    We typically don't use shortened variable names unless it is very clear 
what they are to be used for.  In this case, reading from top to bottom of this 
function, I don't know what `ln` is supposed to signify until I look deeper 
into the function.  We should probably call this variable `line_number` 
instead.  Also, we should probably put spaces around the equal sign as before, 
    line_number = 1

support/mesos-style.py (line 43)

    This will not work with `re.match()`. As is, this will only match if the 
non-ascii character is the first character on the line.  Instead, you should 
probably be using `re.search()`.
    Also, the variable `m` should probably be renamed `match` and spaces should 
be added around the `=`.

support/mesos-style.py (line 45)

    To make things more readable, we typically use python's `.format()` 
function to format strings rather than manually concatenate them together e.g.:
    "Non ascii character found in {path} "
    "(Ln: {line_number}, Pos: {position}): {line}".\
            position=str(match.start() + 1),
    **Note**: We typically **don't** put spaces around the `=` when assigning 
keyword arguments in parameters.

support/mesos-style.py (lines 145 - 148)

    I would probably reorganize this to do the checks first and the linting 
last. Assuming the name `check_encoding()` as mentioned in a previous comment, 
I would probably reorder this as:
    license_errors = check_license_header(filtered_candidates_set)
    encoding_errors = check_encoding(list(filtered_candidates_set))
    lint_errors = run_lint(list(filtered_candidates_set))
    total_errors = license_errors + encoding_errors + lint_errors

- Kevin Klues

On April 2, 2016, 1:41 a.m., Yong Tang wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> -----------------------------------------------------------
> (Updated April 2, 2016, 1:41 a.m.)
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Bernd 
> Mathiske, haosdent huang, Kevin Klues, Neil Conway, Vinod Kone, and Deshi 
> Xiao.
> Bugs: MESOS-4033
>     https://issues.apache.org/jira/browse/MESOS-4033
> Repository: mesos
> Description
> -------
> This review request tries to add addition check in mesos-style.pl
> for checking non-ascii characters. It scans .cpp, .hpp, .cc, .h
> files and report error if non-ascii characters exists.
> As part of this review request, two non-ascii characters are identified
> in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
> accordingly.
> Note: .md scan is skipped based on feedback from review request.
> Diffs
> -----
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   support/mesos-style.py 13616065ebe07ca401b385716d9b723f65bb2162 
> Diff: https://reviews.apache.org/r/45033/diff/
> Testing
> -------
> Tested manually and found two non ascii characters in docs/versioning.md 
> (fixed as part of this review request).
> Thanks,
> Yong Tang

Reply via email to