Refactor the usage of $properties param to $args to align with WordPress usage

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.

[gziolo]

Follow up for:

• dev!: handle property registration inside WP_Ability #54

More details in this #54 (comment). The most relevant part from @justlevine:

My takeaway from this conversation is that we're good not caring about that last point (between overloading and the filter if someone really, realy thinks shimming a DTO or VO into this api is a good idea, they have ways), so if y'all don't think it's outweighed by the first two (I don't), I'll use prepare_*( array<string,mixed> ): array<valid-shape> and just throw.

This PR refactors the usage of $properties to $args to reflect the conversation. As part of the refactoring, the definition for ability registration was also updated to remove the optionality of $args because to work correctly, there needs to be at least ability_class passed, or more frequently, three fields: label, description, and execute_callback.

Entire codebase was slightly refactored to follow the convention agreed upon.

[gziolo]

@justlevine, I cherry-picked your branch and continued refactoring. I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties. However, it's a minor thing that I wanted to further discuss. Otherwise, I tried to go with args in all other places instead of properties, including unit tests.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.22%. Comparing base (21af812) to head (79269ed).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
includes/abilities-api/class-wp-ability.php	43.75%	18 Missing ⚠️
...udes/abilities-api/class-wp-abilities-registry.php	83.33%	1 Missing ⚠️
includes/bootstrap.php	0.00%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## trunk WordPress/abilities-api#59 +/- ##
============================================
- Coverage 84.28% 84.22% -0.07% 
Complexity 96 96 ============================================
Files 8 8 Lines 509 507 -2 ============================================
- Hits 429 427 -2 
Misses 80 80 

Flag	Coverage Δ
unit	84.22% <50.00%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justlevine]

I'm inclined to go with prepare_properties( array $args ) because the result of that method is used to set class properties.

Semantically, I'm good either way (do you "prepare the args" to be used as class properties or do you "prepare properties" so they can be used?)

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to keep the developer DX and terminology intuitive.

But filters don't need to match the semantics for direct class overloading, and they also have a much easier core deprecation path if we want to change the name compared to a protected function *, so 🤷

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: justlevine <justlevine@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: emdashcodes <emdashcodes@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[gziolo]

Only reason I went with prepare_args() is because I'm assuming we'd want the eventual filter to use args ( e.g. return return apply_filters( 'ability_args', $args, $this->name ), or even ability_prepare_args, or whatever naming semantics ), to WordPress/ai#40.

Right, both names are fine here in that context. The filter name doesn't need to follow the method name, so we can be flexible.

[gziolo]

I verified the changes in the WP core env (WordPress/wordpress-develop#9410), and applied some small tweaks to ensure CI is green there, too.

I also set the version for the plugin and Composer package to v0.2.0 to account for the fact that we slightly modified the public API params – $args are no longer optional, which better reflects the real usage.

[gziolo]

@justlevine or @emdashcodes. It looks like I can no longer merge without approval from someone else. I would appreciate a final check from one of you.

[emdashcodes]

Just had a chance to look. 👍

[justlevine]

I also set the version for the plugin and Composer package to v0.2.0

In the future, would we be able to use @since n.e.x.t. and save pre-release versioning for a separate PR / release step?

1. It prevents us from missing other things that need to be versioned (e.g. the constant, a docs ref too iirc)
2. It prevents trunk from lying to us or our LLMs. There's no v0.2.0 until we cut it.

[gziolo]

In the future, would we be able to use @SInCE n.e.x.t. and save pre-release versioning for a separate PR / release step?

Is it something that could be automated with a GitHub action triggered when a new release is created through GitHub UI?

[justlevine]

Yes it is, though if there isn't already FOSSable prior art somewhere, it might not make sense to waste time on a solution for this repo alone over the next six weeks.