Page MenuHomePhabricator

Remove "HeraldRepetitionPolicyConfig" and hide storage details inside HeraldRule
ClosedPublic

Authored by epriestley on Jan 25 2018, 4:10 AM.
Tags
None
Referenced Files
F13139487: D18926.diff
Fri, May 3, 2:50 AM
Unknown Object (File)
Mon, Apr 29, 2:30 PM
Unknown Object (File)
Wed, Apr 24, 10:32 PM
Unknown Object (File)
Fri, Apr 19, 7:10 PM
Unknown Object (File)
Thu, Apr 11, 7:35 AM
Unknown Object (File)
Thu, Apr 11, 1:12 AM
Unknown Object (File)
Apr 2 2024, 4:51 PM
Unknown Object (File)
Apr 2 2024, 4:51 PM
Subscribers
None

Details

Summary

Depends on D18925. Ref T13048. Currently, HeraldRule stores policies as integers (0 or 1) in the database.

The application tries to mostly use strings ("first", "every"), but doesn't do a good job of hiding the fact that the values are integers deeper in the stack. So we end up with a lot of code like this:

$stored_int_value = $rule->getRepetitionPolicy();
$equivalent_string = HeraldRepetitionPolicyConfig::getString($stored_int_value);
$is_first = ($equivalent_string === HeraldRepetitionPolicyConfig::FIRST);

This happens in several places and is generally awful. Replace it with:

$is_first = $rule->isRepeatFirst();

To do this, merge HeraldRepetitionPolicyConfig into HeraldRule and hide all the mess inside the methods.

(This may let us just get rid of the integers in a later change, although I'm not sure I want to commit to that.)

Test Plan
  • Grepped for HeraldRepetitionPolicyConfig, no more hits.
  • Grepped for setRepetitionPolicy(...) and getRepetitionPolicy(...). There are no remaining callers outside of HeraldRule.
  • Browed and edited several rules. I'll vet this more convincingly after adding the new repetition rule.

Diff Detail

Repository
rP Phabricator
Branch
herald3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 19175
Build 25893: Run Core Tests
Build 25892: arc lint + arc unit