Page MenuHomePhabricator
Differential D13937

Add a ternary expression spacing linter rule
AcceptedPublic
Actions

Authored by joshuaspence on Aug 19 2015, 9:07 AM.
Tags
None
Referenced Files
F14081190: D13937.diff
Fri, Nov 22, 6:43 PM
Unknown Object (File)
Sun, Nov 10, 1:48 AM
Unknown Object (File)
Wed, Nov 6, 6:25 PM
Unknown Object (File)
Wed, Nov 6, 6:25 PM
Unknown Object (File)
Thu, Oct 31, 11:52 PM
Unknown Object (File)
Oct 22 2024, 5:35 AM
Unknown Object (File)
Oct 20 2024, 4:03 PM
Unknown Object (File)
Oct 16 2024, 2:48 PM
View All Files
Subscribers
Korvin

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

Unit TestsFailed
View All

TimeTest
178 msArcanistCppcheckLinterTestCase::Unknown Unit Message ("")
332 msArcanistPuppetLintLinterTestCase::Unknown Unit Message ("")
9 msArcanistRubyLinterTestCase::Unknown Unit Message ("")
17 msArcanistAliasFunctionXHPASTLinterRuleTestCase::Unknown Unit Message ("")
15 msArcanistAliasFunctionXHPASTLinterRuleTestCase::Unknown Unit Message ("")
View Full Test Results (3 Failed · 270 Passed · 46 Skipped)

Event Timeline

joshuaspence updated this revision to Diff 33642.Aug 19 2015, 9:07 AM
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.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 19 2015, 9:07 AM
Herald added a subscriber: Korvin. · View Herald Transcript
joshuaspence added a comment.Aug 19 2015, 10:16 PM

@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.
joshuaspence mentioned this in D13959: Create operator nodes in ternary expressions.Aug 22 2015, 1:23 AM
epriestley accepted this revision.Aug 27 2015, 11:26 AM
epriestley edited edge metadata.
This revision is now accepted and ready to land.Aug 27 2015, 11:26 AM
epriestley added a comment.Aug 27 2015, 11:29 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)?

epriestley added inline comments.Aug 27 2015, 11:30 AM
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.)

joshuaspence mentioned this in rPHU33975b2bc75c: Create operator nodes in ternary expressions.Aug 27 2015, 12:07 PM
joshuaspence updated this revision to Diff 35131.Nov 19 2015, 9:15 PM
joshuaspence edited edge metadata.

Rebase

Revision Contents
Changeset List

Diff 35131

View Options

src/__phutil_library_map__.php

Loading...
View Options

src/lint/linter/xhpast/rules/ArcanistTernaryExpressionSpacingXHPASTLinterRule.php

Loading...
View Options

src/lint/linter/xhpast/rules/__tests__/ArcanistTernaryExpressionSpacingXHPASTLinterRuleTestCase.php

Loading...
View Options

src/lint/linter/xhpast/rules/__tests__/ternary-expression-spacing/ternary-expression-spacing.lint-test

Loading...
Phabricator · Built by Phacility