Page MenuHomePhabricator

Don't explicitly name abstract base classes
ClosedPublic

Authored by joshuaspence on Jul 19 2014, 6:08 AM.
Tags
None
Referenced Files
F11050234: D9983.id24043.diff
Fri, Aug 19, 8:26 PM
Unknown Object (File)
Fri, Aug 12, 7:52 AM
Unknown Object (File)
Thu, Aug 11, 3:24 AM
Unknown Object (File)
Thu, Jul 28, 3:00 PM
Unknown Object (File)
Jun 30 2022, 1:46 PM
Unknown Object (File)
Jun 30 2022, 1:46 PM
Unknown Object (File)
Jun 30 2022, 9:49 AM
Unknown Object (File)
Jun 30 2022, 9:49 AM

Details

Summary

Ref T5655. It is superfluous to include "base" in the name of an abstract base class. Furthermore, it is not done consistently within the code base.

In order to retain compatibility with external code, I have kept the ArcanistBaseWorkflow class (which trivially extends from ArcanistWorkflow), but it is now deprecated and should output a warning message. Similarly for ArcanistBaseUnitTestEngine.

Test Plan

Created a workflow which extends from ArcanistBaseWorkflow. Executed the workflow and saw a deprecation warning.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence retitled this revision from to Don't explicit name abstract base classes.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
joshuaspence retitled this revision from Don't explicit name abstract base classes to Don't explicitly name abstract base classes.Jul 19 2014, 6:11 AM
joshuaspence edited edge metadata.

There are a couple of classes remaining, which I wasn't sure what to do with:

  • ArcanistBaseXHPASTLinter (ArcanistXHPASTLinter already exists)
  • PhabricatorBaseEnglishTranslation (PhabricatorEnglishTranslation already exists)
  • PhabricatorSearchBaseController (PhabricatorSearchController already exists)
  • BaseHTTPFuture (HTTPFuture already exists)

I think renaming ArcanistBaseWorkflow without warning will break too many installs. Can we provide a migration path instead? Specifically, add a new ArcanistBaseWorkflow class which extends ArcanistWorkflow and either overrides some method (if convenient) or gets an instanceof check during startup, causing emission of a deprecation warning?

joshuaspence edited edge metadata.
  • Add ArcanistBaseWorkflow but raise a deprecation warning
joshuaspence edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.
This revision is now accepted and ready to land.Jul 21 2014, 2:00 PM
src/workflow/ArcanistWorkflow.php
92

Oh, this will raise ArcanistWorkflow, not ArcanistBaseWorkflow.

src/workflow/ArcanistWorkflow.php
92

Oh whoops.. I had this code in ArcanistBaseWorkflow and then blindly moved it here.

I wonder if we should do the same for ArcanistBaseUnitTestEngine?

Yeah, that might be worthwhile.

joshuaspence edited edge metadata.
  • Fix deprecation message
  • Add a trivial ArcanistBaseUnitTestEngine implementation