mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).
Date Sun, 03 Apr 2016 16:21:10 GMT

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



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:
https://github.com/klueska-mesosphere/mesos/commits/r/yongtang/non-ascii

Thanks!


support/mesos-style.py (line 34)
<https://reviews.apache.org/r/45033/#comment189791>

    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)
<https://reviews.apache.org/r/45033/#comment189792>

    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)
<https://reviews.apache.org/r/45033/#comment189793>

    We typically put spaces between our variable names and our equal signs. e.g.:
    
    ```
    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)
<https://reviews.apache.org/r/45033/#comment189798>

    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)
<https://reviews.apache.org/r/45033/#comment189795>

    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, e.g.:
    
    ```
    line_number = 1
    ```



support/mesos-style.py (line 43)
<https://reviews.apache.org/r/45033/#comment189796>

    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)
<https://reviews.apache.org/r/45033/#comment189797>

    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}".\
        format(
            path=path,
            line_number=line_number,
            position=str(match.start() + 1),
            line=line))
    ```
    
    **Note**: We typically **don't** put spaces around the `=` when assigning keyword arguments
in parameters.



support/mesos-style.py (lines 145 - 148)
<https://reviews.apache.org/r/45033/#comment189799>

    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
> 
>


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