-
Notifications
You must be signed in to change notification settings - Fork 928
Re organize ai code #22440
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: trunk
Are you sure you want to change the base?
Re organize ai code #22440
Conversation
Pull Request Test Coverage Report for Build 44881b69ddc2a6cdc14ba46e26e0e855153f4d93Details
💛 - Coveralls |
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
77469ce
to
b982687
Compare
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
@@ -1,8 +1,9 @@ | |||
<?php |
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.
We disable this rules in many files.
can we consider to extend maxSegments in phpcs.xml?
<rule ref="Yoast.NamingConventions.NamespaceName.TooLong">
<properties>
<property name="maxSegments" value="5"/>
</properties>
</rule>
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.
Yes, there's a discussion about this here.
* | ||
* @var array<Consent_Endpoint_Interface> | ||
*/ | ||
private $endpoints; |
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.
Can this filed be removed taking into account the the same filed is present in Endpoints_Repository and DI will take into account constructor definition but not the internal implementation ?
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.
Fixed here.
* | ||
* @var array<Free_Sparks_Endpoint_Interface> | ||
*/ | ||
private $endpoints; |
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.
Can we remove this field ?
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.
Fixed here.
* | ||
* @var array<Generator_Endpoint_Interface> | ||
*/ | ||
private $endpoints; |
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.
Can we remove this field ?
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.
Fixed here.
@pls78 please resolve merge conflicts. |
…-related modules
This is done so that our DI container knows which endpoints to gather in each integration through the endpoints repo
b982687
to
2d5c5ad
Compare
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
Context
AI Optimize
andAI Summarize
features.Summary
This PR can be summarized in the following changelog entry:
ai
parent directory and consequently losing theAI_
namespace/class prefixRelevant technical choices:
AI_
prefix and are now under anai
parent directory.Endpoint_List
andEndpoints_Repository
classes have been moved tosrc/routes/endpoint
directory, so that they can be re-used throughout the codebase and not being re-declared each time.Consent_Endpoint_Interface
,Free_Sparks_Endpoint_Interface
,Generator_Endpoint_Interface
have been introduced to let our DI container know which endpoints need to be gathered by the respective repository. As a result, they are attached to the respective integration window object in theendpoint
property (exception made for the Free Sparks one, which is bundled in thewpseoAiGenerator
window object.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #632