-
Notifications
You must be signed in to change notification settings - Fork 31
Move new annotations property under meta and update docs
#104
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
Conversation
|
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. |
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.
Pull Request Overview
This PR expands documentation for the new annotations property on WP_Ability objects, providing comprehensive details about its structure and usage across multiple documentation files.
- Added detailed documentation for the
annotationsproperty structure and its four supported keys - Included practical examples showing how to use annotations in ability registration
- Updated REST API documentation to show the annotations property in JSON responses
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/3.registering-abilities.md | Added detailed documentation for the annotations parameter and updated code examples to demonstrate usage |
| docs/4.using-abilities.md | Added example code showing how to retrieve annotations from an ability object |
| docs/5.rest-api.md | Updated JSON response examples to include the annotations property structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #104 +/- ##
============================================
- Coverage 86.27% 86.24% -0.04%
+ Complexity 111 110 -1
============================================
Files 16 16
Lines 809 807 -2
Branches 85 85
============================================
- Hits 698 696 -2
Misses 111 111
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:
|
|
As a quick note, I want to make sure that we're documenting the decision elsewhere that all annotations should be snake case. |
|
Sure, snake case usage is a coding style convention in WordPress and we discussed that yesterday with shared understanding it’s not always convenient. I will link later to the thread. |
|
Depends now on #106. |
82f888b to
379d642
Compare
annotations propertyannotations property under meta and update docs
379d642 to
c2a5f69
Compare
fee5c3c to
207146e
Compare
c2a5f69 to
96a3319
Compare
96a3319 to
da2d010
Compare
da2d010 to
b2f6518
Compare
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.
Pull Request Overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Thanks for doing this, @gziolo! I left one question, but for me it's not a blocker to merging, so I'll go ahead and approve!
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.
What is the typical WordPress approach to redundancy? I personally wouldn't mind having a getter and setter for annotations, while still storing them in meta. Yes, one could use get_meta() to retrieve the annotations, but for built-in meta it's nice to have more declarative methods.
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 very much open for adding a getter to simplify access to individual annotations. I don’t think WordPress core has many good reference points in that regard because older code is often composed of functions chained together. I can spin a follow up PR. To confirm we are on the same page, are we talking about something like:
$this->get_annotation( ‘readonly’ );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.
Yup! Exactly. Just convenience methods.
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.
If possible, I'd like to save convenience methods for 7.0.
I know nested arrays are messy so I see why we might want to make an exception for annotations (at least in their current shape), but FWIW @gziolo did just introduce ::get_meta_item() in #107, so the method would only be at the convenience of $ability->get_meta_item( 'annotations' )['readonly']
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 don't see a reason to hesitate on these things unless you foresee something like annotations going away or moving.
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.
@JasonTheAdams more details in the comment and subsequent discussion I linked to above in #62 but tl;dr I expect at minimum "annotation" to be renamed to something that makes sense semanticaly in non-MCP contexts, as well as the shape/location/keys for some of the meta based on what gets decided in #106 . Hopefully in time for 6.9 so it too doesn't become cumulative tech debt.
But also if I couldn't think of ways to possibly improve the semantics/dx, I'd hope the fact that we're not dogfooding the function internally would be reason enough to pause. We're already navelgazing these props into existence, it's counterproductive to keep expanding our API with compounding layers of conjecture, and slows down our ability to iterate since once something is in core we need to work around it forever.
(Like the linked comment it's not a big deal if we add it now in time for beta1 and then remove during core review. But I do feel it's pretty likely this method will be obviated before rc )
Follow-up #99.
Addresses the feedback from #106.
Branch is currently based on #107 to minimize the need for merge conflict resolution. Both PRs move properties tometa, so similar lines need to be updated.Refactoring ensures that
annotationsis no longer a top-level property in theWP_Abilityclass. Instead, it is now its group inside themetaproperty. This is similar to the previous changes applied to theshow_in_restproperty. As part of the refactoringget_annotations()public method got removed in favor ofget_meta_item( 'annotation' )calls.This PR expands on the documentation change necessary for understanding how the
annotationproperty insidemetaproperty onWP_Abilityshould be used.