-
Notifications
You must be signed in to change notification settings - Fork 28
Adds Abilities Explorer #63
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #63 +/- ##
=============================================
- Coverage 48.48% 21.22% -27.26%
- Complexity 45 219 +174
=============================================
Files 6 13 +7
Lines 198 914 +716
=============================================
+ Hits 96 194 +98
- Misses 102 720 +618
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:
|
mindctrl
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.
I'm digging this so far. I left a question or two and some feedback, mostly about coding standards.
| * | ||
| * @since 0.1.0 | ||
| */ | ||
| class Admin_Page { |
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.
Similar to other classes, I think we could type arguments and returns.
jonathanbossenger
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.
I think this will be a great feature for the plugin. Left a bunch of suggestions, happy to discuss any of them further.
| * @since 0.1.0 | ||
| * @var string | ||
| */ | ||
| const VERSION = '1.0.0'; |
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.
Given that these two constants seem to only be used internally, can we make these private?
| * @since 0.1.0 | ||
| */ | ||
| private function render_detail_view() { | ||
| $ability_slug = isset( $_GET['ability'] ) ? sanitize_text_field( wp_unslash( $_GET['ability'] ) ) : ''; |
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.
This code is duplicated again at line 316. Is it worth calling this once higher up the chain, and then passing the $ability_slug to the relevant render_ method?
| } else { | ||
| wp_send_json_error( | ||
| array( | ||
| 'message' => isset( $result['error'] ) ? $result['error'] : __( 'Unknown error occurred.', 'ai' ), |
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.
This could also be written as
'message' => $result['error'] ?? __( 'Unknown error occurred.', 'ai' ),
'trace' => $result['trace'] ?? null,
includes/Features/Abilities_Explorer/assets/js/abilities-explorer.js
Outdated
Show resolved
Hide resolved
| $button.html(aiAbilityExplorer.strings.invoking + '<span class="ability-loading"></span>'); | ||
|
|
||
| // Make AJAX request | ||
| $.ajax({ |
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.
As mentioned elsewhere, this is a perfect example of somewhere this plugin could use a custom Ability and the JavaScript client.
|
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. |
| // Check if Abilities API is available | ||
| if ( ! $this->check_abilities_api() ) { |
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.
Because we include the Abilities API via composer, I don't think we need this check as we can assume it's always available
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.
Also, this plugin requires 6.9 which will include the Abilities API, so no checks needed.
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.
So ideally these assets (JS and CSS files) should be handled like other assets will be in this plugin. Meaning I would expect these to live within the top-level src directory and be compiled by wp-scripts. This is a pattern we're still figuring out but can see how I'm approaching it in this PR: https://github.com/WordPress/ai/pull/83/files
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.
The PRs for the Asset_Loader have been merged, so we can do this now after we merge trunk into this branch!
| } | ||
|
|
||
| // Enqueue styles | ||
| wp_enqueue_style( |
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.
So this works fine but wanted to note we are adding (assuming it gets approval 😝) an Asset_Loader class here: https://github.com/WordPress/ai/pull/83/files#diff-9b683a19a4d64a079b42ade32de8038f235d7b12f619ae623796f7a9eff667cd, that standardizes how CSS and JS files are enqueued and how scripts are localized. The benefit here is it handles pulling versions and dependencies from the .asset.php files wp-scripts produces, noting that will only work here if this comment is followed: https://github.com/WordPress/ai/pull/63/files#r2529040376
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.
The PRs for the Asset_Loader have been merged, so we can do this now after we merge trunk into this branch!
|
PS: our CI/CD assumes trunk-based development so CI/CD isnt running on Here's the PHPStan output from this branch atm. The missing array shapes are likely noise, but the type conflicts are likely pointing to real bugs that need addressing (h/t @jeffpaul ) ------ ------------------------------------------------------------------------------------------
Line includes/Features/Abilities_Explorer/Abilities_Explorer.php
------ ------------------------------------------------------------------------------------------
:30 Constant WordPress\AI\Features\Abilities_Explorer\Abilities_Explorer::VERSION is unused.
🪪 classConstant.unused
💡 See: https://phpstan.org/developing-extensions/always-used-class-constants
------ ------------------------------------------------------------------------------------------
------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------
Line includes/Features/Abilities_Explorer/Ability_Handler.php
------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------
:33 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::get_all_abilities() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:51 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::get_ability() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:73 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::format_abilities() has parameter $abilities with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:73 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::format_abilities() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:95 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::format_single_ability() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:130 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::detect_provider() has parameter $meta with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:165 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::invoke_ability() has parameter $input with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:165 PHPDoc tag @return with type array is incompatible with native type string.
🪪 return.phpDocType
:167 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::invoke_ability() should return string but returns array<string, string|false>.
🪪 return.type
:176 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::invoke_ability() should return string but returns array<string, string|false>.
🪪 return.type
:192 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::invoke_ability() should return string but returns array<string, mixed>.
🪪 return.type
:200 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::invoke_ability() should return string but returns array<string, mixed>.
🪪 return.type
:215 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::validate_input() has parameter $input with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:215 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::validate_input() has parameter $schema with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:215 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::validate_input() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:290 Method WordPress\AI\Features\Abilities_Explorer\Ability_Handler::get_statistics() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
------ -------------------------------------------------------------------------------------------------------------------------------------------------------------------
------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Line includes/Features/Abilities_Explorer/Ability_Table.php
------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
:52 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::get_columns() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:69 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::get_sortable_columns() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:82 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::prepare_items() has no return type specified.
🪪 missingType.return
:139 Parameter #1 $args of method WP_List_Table::set_pagination_args() expects array{total_items?: int, total_pages?: int, per_page?: int}, array{total_items: int<0, max>, per_page: 20, total_pages: float}
given.
🪪 argument.type
💡 Offset 'total_pages' (int) does not accept type float.
:158 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_default() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:170 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_cb() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:185 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_name() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:211 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_slug() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:226 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_provider() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:245 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::column_actions() has parameter $item with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:280 Method WordPress\AI\Features\Abilities_Explorer\Ability_Table::extra_tablenav() has no return type specified.
🪪 missingType.return
------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------
Line includes/Features/Abilities_Explorer/Admin_Page.php
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------
:100 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::render_statistics() has no return type specified.
🪪 missingType.return
:133 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::render_list_view() has no return type specified.
🪪 missingType.return
:153 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::render_detail_view() has no return type specified.
🪪 missingType.return
:209 Parameter #1 $text of function esc_html expects string, string|false given.
🪪 argument.type
:217 Parameter #1 $text of function esc_html expects string, string|false given.
🪪 argument.type
:224 Parameter #1 $text of function esc_html expects string, string|false given.
🪪 argument.type
:235 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::render_test_runner() has no return type specified.
🪪 missingType.return
:303 Parameter #1 $text of function esc_textarea expects string, string|false given.
🪪 argument.type
:328 Parameter #1 $text of function esc_html expects string, string|false given.
🪪 argument.type
:347 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::generate_example_input() has parameter $schema with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:347 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::generate_example_input() return type has no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:369 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::get_example_value() has parameter $prop_schema with no value type specified in iterable type array.
🪪 missingType.iterableValue
💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
:402 Method WordPress\AI\Features\Abilities_Explorer\Admin_Page::ajax_invoke_ability() has no return type specified.
🪪 missingType.return
:455 Offset 'success' does not exist on string.
🪪 offsetAccess.notFound
:459 Offset 'data' does not exist on string.
🪪 offsetAccess.notFound
:465 Offset 'error' on string in isset() does not exist.
🪪 isset.offset
:466 Offset 'trace' on string in isset() does not exist.
🪪 isset.offset
------ -----------------------------------------------------------------------------------------------------------------------------------------------------------------
[ERROR] Found 45 errors |
What?
This is an initial very raw version PR to add an Abilities Explorer to the plugin. Responds to #62.
For now this adds a new menu item where you can see the abilities. You can also view or test them
Requires 6.9 beta to work.
Why?
Responds to the proposal in #62 and focuses on making them visible whilst providing a learning opportunity.
How?
This is a simpler implementation, adding a menu item and keeping it totally seperate as proposed for features.
Testing Instructions
Once using branch and installed you should be able to access it from Abilities menu item.
Screenshots or screencast
Note: as this is a work in progress going to keep as a draft until both decided on as direction and had some robust code review. This is being created from an experiment so there likely are code dragons - let’s slay them!