-
Notifications
You must be signed in to change notification settings - Fork 43
Change experiment initialization method from register() to init() #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Updated method names from `register()` to `init()` across various classes and interfaces to better reflect their purpose. This change enhances clarity in the codebase regarding the initialization of experiments and their hooks.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #159 +/- ##
==========================================
Coverage 46.89% 46.89%
Complexity 208 208
==========================================
Files 19 19
Lines 1271 1271
==========================================
Hits 596 596
Misses 675 675
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
"Plugin Check" workflow is failing due to an issue reported at #160 |
|
#160 has been merged in, so hopefully an update from |
JasonTheAdams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor wording suggestion, but otherwise looks good!
| public function register(): void { | ||
| // Register your hooks here | ||
| public function init(): void { | ||
| // Initialize your hooks here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Initialize your hooks here | |
| // Initializes your experiment. Use this to set up your needed hooks. |
|
I very much disagree with the proposed change, and the premise that Registration != instantiation, and Moreover the conflation between the two leads to unnecessary headaches around lifecycle and testing, and we shouldnt be registering hooks in an If we want to add an Alternatively, if you think |
justlevine
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as Request changes to ensure a conversation is had #159 (comment) - obvs is the majority disagree I'm good with being overruled 🙇
|
@justlevine Appreciate the feedback — I agree with your reasoning. I’m in favor of using
I’d love to hear more thoughts before moving ahead with any changes on this matter. |
What?
Refactors the experiment lifecycle method from
register()toinit()across the entire codebase to better reflect its purpose of initializing experiments.Closes #145
Why?
The term "register" was misleading as it suggested the experiments were being registered into a system, when in reality this method is called after registration to initialize the experiment's hooks and functionality. The method
init()more accurately describes what the method does - it initializes the experiment by setting up WordPress hooks, filters, and actions when the experiment is enabled and loaded.This improves code clarity and aligns the method naming with its actual behavior in the experiment lifecycle:
Experiment_Registry::register()Experiment::init()when they're loadedHow?
Experimentinterface to declareinit()instead ofregister()Abstract_Experimentabstract class to declareabstract public function init()Experiment_Loader::initialize_experiments()to callinit()Testing Instructions
npm run test:phpTesting Instructions for Keyboard
Not applicable - this is an internal refactoring with no UI changes.