Issue #9042: Report incorrect number of spaces around => in the style checker

[simartin]

Fixes #9042

[highfive]

Warning

• These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

[wafflespeanut]

This looks nice. Only one nit :)

---

Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

python/tidy.py, line 317 [r1] (raw file):
Nit: I suppose we could just use line.find since we're not really doing a complex regex match. Moreover, if we make use of find we could just use the index to check for spaces (by this way, we can also avoid multiple searches)

---

Comments from the review on Reviewable.io

[simartin]

python/tidy.py, line 317 [r1] (raw file):
Yes indeed; update is coming up

---

Comments from the review on Reviewable.io

[simartin]

Thanks for the review. I've replaced the first test by a call to find, but I've kept the rest as regexp matching for readability (catching all kinds of whitespaces won't be very readable). I also check for whitespace before => because patterns like "Err(_)=>" are not caught

---

Comments from the review on Reviewable.io

[wafflespeanut]

Review status: 6 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

python/tidy.py, line 317 [r2] (raw file):
Since we don't actually have to check for all spaces, why can't we do this?

match_idx = line.find('=>')
if match != -1:
    if line[match_idx - 1] != ' ':
        # missing space ...
    elif line[match_idx + 2] != ' ':
        # missing space ...
    elif line[match_idx + 3] == ' ':
        # extra space ...

This also avoids multiple searches, right?

---

Comments from the review on Reviewable.io

[simartin]

python/tidy.py, line 317 [r2] (raw file):
It wouldn't work if we have for instance \t instead of spaces, would it?

---

Comments from the review on Reviewable.io

[wafflespeanut]

Review status: 6 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

python/tidy.py, line 317 [r2] (raw file):
No, but we should enforce making use of space and newlines (no tabs!). I just realized that we already use newlines in match arms at various places. So, the only additional change would be

match_idx = line.find('=>')
if match != -1:
    if line[match_idx - 1] != ' ':
        # missing space ...
    elif line[match_idx + 2] != ' ':
        if line[match_idx + 2] != '\n'
            # missing space ...
    elif line[match_idx + 3] == ' ':
        # extra space ...

I'm sorry that I'm being nitpicky here. The reason I'm obsessed about re.search is because we have about 8k lines of code in Servo that has =>, and this is somewhat expensive. So, we should try to avoid it for cases where we have an alternative.

Well, I agree that there are plenty of regex searches in tidy already. I'm planning to refactor it someday soon. That said, I'll leave this decision to someone else (I'm probably biased about this :P)

---

Comments from the review on Reviewable.io

[simartin]

python/tidy.py, line 317 [r2] (raw file):
Makes sense, thanks. PR update will come shortly

---

Comments from the review on Reviewable.io

[jdm]

I believe

    elif line[match_idx + 2] != ' ':
        if line[match_idx + 2] != '\n'

could also be written as:

    elif line[match_idx + 2] not in [' ', '\n']:

edit: ignore this, since the code in question isn't even present in the PR.

[jdm]

@bors-servo: delegate=Wafflespeanut

[wafflespeanut]

@jdm Yeah, that works. I'm pretty sure I thought of that back then, but I now forget why I chose not to use it. (sigh)

[wafflespeanut]

Review status: 6 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

python/tidy.py, line 319 [r3] (raw file):
We don't want this arrow_pos > 0, because I'm pretty sure almost all the cases are indented (and so, the lines always begin with spaces). But, if you really insist, I suggest the pythonic way - if arrow_pos and ... :)

---

python/tidy.py, line 321 [r3] (raw file):
We really don't want this arrow_pos + 2, because the end of the line will definitely be a newline or a space (that's how we split the lines). Also, you forgot to include newline here. See @jdm's comment...

---

python/tidy.py, line 323 [r3] (raw file):
This is okay, but I now realize that we've used len(line) about 4 times in check_rust. So, let's declare a line_length at the start of the iteration, and replace all those lengths.

---

Comments from the review on Reviewable.io

[simartin]

python/tidy.py, line 319 [r3] (raw file):
Heading whitespaces are removed there: line = original_line.strip() (currently line 250), so line[arrow_pos - 1] might be not where we expect.

---

Comments from the review on Reviewable.io

[simartin]

python/tidy.py, line 321 [r3] (raw file):
I don't think it's true since line is original_line stripped, so \n and trailing spaces are removed in line already

---

Comments from the review on Reviewable.io

[wafflespeanut]

The PR looks great!

---

Reviewed 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

python/tidy.py, line 319 [r3] (raw file):
Oops, didn't notice that, sorry :)

---

Comments from the review on Reviewable.io

[wafflespeanut]

@bors-servo r+

Thanks!

[wafflespeanut]

(sigh)

[Manishearth]

@bors-servo r=Wafflespeanut

[Manishearth]

@bors-servo r=Wafflespeanut

[bors-servo]

📌 Commit cec661f has been approved by Wafflespeanut

[bors-servo]

⌛ Testing commit cec661f with merge f77c792...

[bors-servo]

💔 Test failed - linux-rel

[KiChjang]

Ran 3737 tests finished in 212.0 seconds.
  • 3736 ran as expected. 709 tests skipped.
  • 1 tests failed unexpectedly

Tests with unexpected results:
  ▶ FAIL [expected PASS] /_mozilla/css/nth_last_of_type_pseudo_a.html
  └   → /_mozilla/css/nth_last_of_type_pseudo_a.html c368d1126a9f5412469b9167a2622a0d43593d96
/_mozilla/css/nth_last_of_type_pseudo_b.html c568e8f044d5faec2a239694ad297caffcb9a00e
Testing c368d1126a9f5412469b9167a2622a0d43593d96 == c568e8f044d5faec2a239694ad297caffcb9a00e

[KiChjang]

@bors-servo retry

• #9063

[bors-servo]

⚡ Previous build results for android, gonk, linux-dev, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt