Page MenuHomePhabricator

Made some additional methods of `ArcanistBaseWorkflow` final.
ClosedPublic

Authored by joshuaspence on Jan 14 2014, 4:24 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 19, 11:24 PM
Unknown Object (File)
Sat, Nov 16, 8:49 AM
Unknown Object (File)
Tue, Nov 12, 12:23 PM
Unknown Object (File)
Fri, Nov 8, 11:03 AM
Unknown Object (File)
Fri, Nov 8, 8:06 AM
Unknown Object (File)
Tue, Nov 5, 6:42 AM
Unknown Object (File)
Sun, Oct 27, 11:00 AM
Unknown Object (File)
Oct 9 2024, 3:19 PM
Tokens
"The World Burns" token, awarded by pieter.

Details

Summary

To me, it seems that these methods should never be overwritten in subclasses.

Test Plan

arc lint and arc unit.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

joshuaspence edited edge metadata.

Made a few other methods final

joshuaspence retitled this revision from Made some additional methods of `ArcanistBaseWorkflow` final to Made some additional methods of `ArcanistBaseWorkflow` final..May 4 2014, 1:12 PM
joshuaspence edited the test plan for this revision. (Show Details)
epriestley edited edge metadata.
epriestley added a subscriber: LegNeato.

@LegNeato, yell if any of these cause issues. One more after this, D7960.

(I can't think up any good reasons a subclass would override these methods.)

src/workflow/ArcanistBaseWorkflow.php
1500

I could imagine something overriding this, adding some $data, and calling the parent, but it should probably wrap it instead if it does.

This revision is now accepted and ready to land.May 18 2014, 6:05 PM
epriestley updated this revision to Diff 21814.

Closed by commit rARCa67b848f4460 (authored by @joshuaspence, committed by @epriestley).

This again broke our arc workflow. I'll look if it is needed though it would be nice to get a revert.

Is there any particular reason why we are locking these down all of a sudden? It seems like if they stay open then it is on us to to track you, which is a lot easier as we have all the info as to what our code does :D

FWIW, this is what broke / why we override:

public function getRepositoryAPI() {
$repository_api = parent::getRepositoryAPI();
if (!($repository_api instanceof ArcanistGitAPI) &&
    !($repository_api instanceof ArcanistMercurialAPI)) {
  throw new ArcanistUsageException(
    'arc cleanup-features is only supported under Git and Mercurial.');
}
return $repository_api;
In D7959#15, @LegNeato wrote:

FWIW, this is what broke / why we override:

public function getRepositoryAPI() {
$repository_api = parent::getRepositoryAPI();
if (!($repository_api instanceof ArcanistGitAPI) &&
    !($repository_api instanceof ArcanistMercurialAPI)) {
  throw new ArcanistUsageException(
    'arc cleanup-features is only supported under Git and Mercurial.');
}
return $repository_api;

There's a much easier way to do that:

protected function getSupportedRevisionControlSystems() {
  return array('git', 'hg');
}