Add a ternary expression spacing linter rule
AcceptedPublic
Actions

Authored by joshuaspence on Aug 19 2015, 9:07 AM.

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 7683
Build 8403: [Placeholder Plan] Wait for 30 Seconds
Build 8402: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	70,232 ms	ArcanistXHPASTLinterTestCase::Unknown Unit Message ("")
	158 ms	ArcanistChmodLinterTestCase::Unknown Unit Message ("")
	34 ms	ArcanistClosureLinterTestCase::Unknown Unit Message ("")
	34 ms	ArcanistCpplintLinterTestCase::Unknown Unit Message ("")
	159 ms	ArcanistFilenameLinterTestCase::Unknown Unit Message ("")
		View Full Test Results (1 Failed · 30 Passed · 27 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

@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

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