Page MenuHomePhabricator

Add a ternary expression spacing linter rule
AcceptedPublic

Authored by joshuaspence on Aug 19 2015, 9:07 AM.
Tags
None
Referenced Files
F14034684: D13937.id33642.diff
Sun, Nov 10, 1:48 AM
F14022615: D13937.id35131.diff
Wed, Nov 6, 6:25 PM
F14022614: D13937.id.diff
Wed, Nov 6, 6:25 PM
F14011406: D13937.id33642.diff
Thu, Oct 31, 11:52 PM
F13990717: D13937.diff
Tue, Oct 22, 5:35 AM
F13984798: D13937.id35131.diff
Sun, Oct 20, 4:03 PM
F13967621: D13937.diff
Wed, Oct 16, 2:48 PM
F13959393: D13937.id.diff
Oct 14 2024, 7:12 PM
Subscribers

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Summary

Adds a linter rule to warn against incorrect ternary expression spacing.

Test Plan

Added test cases... this still needs some work.

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 8941
Build 10490: Run Core Tests
Build 10489: arc lint + arc unit

Event Timeline

joshuaspence retitled this revision from to Adda ternary expression spacing linter rule.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.

@epriestley, this linter rule would be much easier if we change the way ternary expressions are parsed (specifically, if we add n_OPERATOR nodes for ? and :)... I'm not sure if this is desirable though.

joshuaspence retitled this revision from Adda ternary expression spacing linter rule to Add a ternary expression spacing linter rule.Aug 19 2015, 10:17 PM
joshuaspence edited edge metadata.
epriestley edited edge metadata.
This revision is now accepted and ready to land.Aug 27 2015, 11:26 AM

The parser change elsewhere seems correct/desirable to me.

Maybe included nested ternary operators in the test case, too, since this will also not get that quite right (the operators will be selected by both the parent and child and we'll raise extra warnings, I think)?

src/lint/linter/__tests__/xhpast/ternary-expression-spacing.lint-test
14 ↗(On Diff #33642)

(We should fix this, of course, but presumably that's straightforward with the operator fix.)