Details

Reviewers

Group Reviewers

Commits

rARC0b1acf0dc02a: Split the `ArcanistXHPASTLinter` into modular rules

Summary

The ArcanistXHPASTLinter class is becoming quite bloated. This diff separates the class into one-class-per-rule, which makes everything much more modular. One downside to this decoupling is that code reuse between linter rules is much more difficult, although this only affects a very small number of linter rules.

There is still some further work that could be done here, but I defer this until a later diff:

Rewrite ArcanistPhutilXHPASTLinter using ArcanistXHPASTLinterRule.
Change the unit tests so that they are truly only testing a single linter rule.
Maybe improve the way in which linter configuration options are handled.
Make it easier to keep track of the linter rule IDs (see T6859).

Test Plan

arc unit

Diff Detail

Repository

rARC Arcanist

Branch

master

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 2838
Build 2842: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 25325.Sep 23 2014, 1:56 PM

joshuaspence retitled this revision from to Split the `ArcanistXHPASTLinter` into modular rules.

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 TranscriptSep 23 2014, 1:56 PM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2612: Diff 25325.Sep 23 2014, 1:57 PM

This is now in a somewhat working state

Harbormaster completed remote builds in B2824: Diff 25686.Oct 13 2014, 8:42 PM

Convert remaining rules to classes

Harbormaster completed remote builds in B2827: Diff 25689.Oct 13 2014, 10:10 PM

Rebase

Harbormaster completed remote builds in B2838: Diff 25705.Oct 15 2014, 9:14 AM

@epriestley, just wanted some initial thoughts before I put more time into this.

Rebase

Seems mostly working now

joshuaspence updated this object.Jan 27 2015, 1:24 PM

joshuaspence edited the test plan for this revision. (Show Details)

Working now

Unfortunately, this really seems to slow the unit tests down, I'm not sure why at the moment.

PASS  1m04s   ArcanistXHPASTLinterTestCase::testLinter

Minor changes

Tidying

Rebase

Remove getEngine()

@epriestley, are you interested in this upstream? It's really difficult to rebase this diff, so I'd rather abandon it now if it's unlikely to be accepted.

Fix derp

Rebase

johnny-bit added a subscriber: johnny-bit.Feb 11 2015, 11:34 AM

Fix weird clone issue

@epriestley, whilst there's no rush on this... is this something that you would consider eventually? (seeing as I didn't file a ticket first)

Yeah, I'm fine with moving forward on this.

ID management is a little unfortunate, but I don't have a cleaner approach at the moment.
If this still causes a big performance hit, getting a before/after profile and figuring out what changed would be nice.

This revision is now accepted and ready to land.May 19 2015, 1:33 PM

joshuaspence added inline comments.May 19 2015, 1:36 PM

src/lint/linter/xhpast/ArcanistXHPASTLinterRule.php
107–116	@epriestley, any ideas on a non-hacky way of doing this?

epriestley added inline comments.May 19 2015, 2:16 PM

src/lint/linter/xhpast/ArcanistXHPASTLinterRule.php
107–116	Oh, gross. Just make them public? This seems like a good justification, and these feel like reasonable methods at the boundary of the API to me.

joshuaspence added a parent revision: D10005: Remove deprecated linter configuration.May 20 2015, 12:09 PM

Rebase

joshuaspence added a parent revision: D12956: Remove "commit hook mode" workarounds for ArcanistXHPASTLinter.May 20 2015, 9:07 PM

Rebase

Change method visibility

One downside to this modularization is that there is no code re-use across linter rules. In the current implementation, it is possible for a single function to raise linter messages of more than one type. This is not very common, however, with only 7 XHPAST linter rules utilizing this code reuse.

joshuaspence mentioned this in D12976: Change visibility of some linter methods.May 22 2015, 7:47 AM

Move a class. Set default severity

joshuaspence mentioned this in rARC9b7c6786cdc9: Change visibility of some linter methods.May 23 2015, 9:45 PM

joshuaspence updated this object.Jun 1 2015, 5:44 AM

joshuaspence added a parent revision: D12976: Change visibility of some linter methods.

joshuaspence updated this object.

This still does seem to be significantly slow (the unit tests run in 1.6 seconds compared to 999 ms before), but I defer any performance optimizations to T7892.

Closed by commit rARC0b1acf0dc02a: Split the `ArcanistXHPASTLinter` into modular rules (authored by joshuaspence, committed by joshuaspence). · Explain WhyJun 1 2015, 5:50 AM

This revision was automatically updated to reflect the committed changes.

joshuaspence mentioned this in D14010: Test XHPAST linter rules in isolation.Aug 30 2015, 9:12 PM

joshuaspence mentioned this in rARC4d512c51d43c: Test XHPAST linter rules in isolation.Nov 18 2015, 9:57 PM

Split the `ArcanistXHPASTLinter` into modular rulesClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 25705

src/__phutil_library_map__.php

src/lint/linter/ArcanistXHPASTLinter.php

src/lint/linter/xhpast/ArcanistXHPASTLinterRule.php

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Split the `ArcanistXHPASTLinter` into modular rules
ClosedPublic
Actions

Revision Contents
Changeset List