Page MenuHomePhabricator

Don't explicitly name abstract base classes
ClosedPublic

Authored by joshuaspence on Jul 19 2014, 6:08 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 11 2024, 9:46 PM
Unknown Object (File)
Jan 20 2024, 2:36 PM
Unknown Object (File)
Jan 7 2024, 4:37 PM
Unknown Object (File)
Jan 6 2024, 3:03 AM
Unknown Object (File)
Jan 3 2024, 10:25 PM
Unknown Object (File)
Jan 3 2024, 6:09 PM
Unknown Object (File)
Dec 30 2023, 4:17 PM
Unknown Object (File)
Dec 26 2023, 6:16 PM

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
Branch
base
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1816
Build 1817: [Placeholder Plan] Wait for 30 Seconds

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