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