Skip to content
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

WPSC_Duplicate_Product Class #1942

Merged
merged 13 commits into from
Jul 10, 2015
Merged

WPSC_Duplicate_Product Class #1942

merged 13 commits into from
Jul 10, 2015

Conversation

benhuson
Copy link
Member

@benhuson benhuson commented Jul 6, 2015

@JustinSainton After doing all the recent updates to the product duplication functionality, I thought It might be better suited to component-ize it into a class.

No-body should really be using the those functions but I've make it back-compatible with the existing functions which we can deprecate if we commit this - happy to add in the deprecated function calls.

What do you think?

@JustinSainton
Copy link
Member

Love the idea! Looks really good so far! Do you want to properly deprecate the old procedural functions in another PR, or just make that commit to this branch? (Also, a few super minor scrutinizer issues).

@benhuson
Copy link
Member Author

benhuson commented Jul 7, 2015

I'll build on this PR and check those issues :)

@benhuson
Copy link
Member Author

benhuson commented Jul 7, 2015

Should I move the deprecated functions to https://github.com/wp-e-commerce/WP-e-Commerce/blob/master/wpsc-core/wpsc-deprecated.php ?

@benhuson benhuson self-assigned this Jul 7, 2015
@JustinSainton
Copy link
Member

Yep!

@benhuson
Copy link
Member Author

benhuson commented Jul 8, 2015

@JustinSainton

Scrutinizer came up with a few minor issues saying "Blank line found after control structure". Are we not supposed to a line break after an if statement if it makes it more readable?

Also it's suggesting for docs to "Consider making the return type a bit more specific; maybe use integer." when it can actual return int OR WP_Error. Is that an issue?

@JustinSainton
Copy link
Member

I think we can fix the control structure spacing (not sure where that standard comes from, but it seems to be inline with core). The integer docs issue - I'd ignore that.

JustinSainton added a commit that referenced this pull request Jul 10, 2015
@JustinSainton JustinSainton merged commit 4d0edf4 into wp-e-commerce:master Jul 10, 2015
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6d6d8c3 on benhuson:Duplicate_Product into ** on wp-e-commerce:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants