wc-smooth-generator icon indicating copy to clipboard operation
wc-smooth-generator copied to clipboard

adding count parameter

Open ovidiul opened this issue 2 years ago • 4 comments

As detailed in issue https://github.com/woocommerce/wc-smooth-generator/issues/76 , a count parameter would be beneficial so that scalable import events could be run from the WordPress cron infrastructure.

All Submissions:

Changes proposed in this Pull Request:

Closes #76 .

How to test the changes in this Pull Request:

  1. Merge PR
  2. Schedule an event from console
wp cron event schedule wc_smooth_generate_object now --type=order --count=10
  1. Check that 10 orders have been created

Other information:

  • [ ] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [ ] Have you written new tests for your changes, as applicable?
  • [ ] Have you successfully run tests with your changes locally?

Changelog entry

Added count parameter to the wc_smooth_generate_object method

FOR PR REVIEWER ONLY:

  • [x] I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

ovidiul avatar Jan 27 '22 05:01 ovidiul

This looks like a powerful, efficient suggestion to me. Thanks for contributing!

I'd only suggest using a do while instead of while, as we guarantee that the generation executes at least once:

function wc_smooth_generate_object( $type, $count = 1 ) {
	$i = 0;
	do {
		$i ++;
		switch ( $type ) {
			case 'order':
				Generator\Order::generate();
				break;
			case 'product':
				Generator\Product::generate();
				break;
			case 'customer':
				Generator\Customer::generate();
				break;
			case 'coupon':
				Generator\Coupon::generate();
				break;
			default:
				return false;
		}
	} while ( $i < intval( $count ) );

	return false;
}

Luc45 avatar Jul 01 '22 14:07 Luc45

@ovidiul would you mind committing the suggestions above, so I can approve this PR and forward this internally to be merged?

Luc45 avatar Jul 01 '22 14:07 Luc45

@Luc45 changes done, thanks for considering this.

ovidiul avatar Jul 01 '22 14:07 ovidiul

Thanks for the change, @ovidiul!

Just out of curiosity, a couple of PHP core developers once warned me about doing computation inside a statement, because the inner workings of PHP cannot guarantee the order of operations at all times, so I usually pre-compute it:

$i = 0;
do {
	$i ++;
} while ( $i < intval( $count ) );

Instead of:

$i = 0;
do {
} while ( $i++ < intval( $count ) );

But I have tested your version and it works fine with all PHP versions, so this seems to be perfectly fine in this case.

I'll be approving this PR now and forwarding this internally to see if we can get this merged soon.

Luc45 avatar Jul 01 '22 16:07 Luc45

Hi @ovidiul,

Apologies not getting to this sooner. It looks like there are some conflicts now. Would you mind resolving those so that we can give this a re-review?

jonathansadowski avatar Sep 23 '22 16:09 jonathansadowski

Hi @jonathansadowski conflict is now resolved.

Thanks

ovidiul avatar Sep 26 '22 06:09 ovidiul

Thanks @ovidiul — merging now 😄

jonathansadowski avatar Sep 26 '22 18:09 jonathansadowski