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

Refactor #405

Merged
merged 6 commits into from
Jun 27, 2020
Merged

Refactor #405

merged 6 commits into from
Jun 27, 2020

Conversation

picandocodigo
Copy link
Owner

Work in progress to do some refactorings, improve Code Climate grading, fix a few things and get this back on track.

// OR relationship, default
if ('yes' === $mode || '' === $mode) return implode(',', $cats);
// Exclude current categories
if ('other' === $mode) return implode(',', array_map(function($cat) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid too many return statements within this method.

list-category-posts.php Show resolved Hide resolved
@picandocodigo picandocodigo force-pushed the refactor branch 2 times, most recently from 4ceaace to 1ac2e91 Compare June 15, 2020 21:55
list-category-posts.php Outdated Show resolved Hide resolved
list-category-posts.php Outdated Show resolved Hide resolved
list-category-posts.php Outdated Show resolved Hide resolved
@picandocodigo picandocodigo force-pushed the refactor branch 4 times, most recently from 1530e30 to 2859496 Compare June 15, 2020 22:12
Repository owner deleted a comment from codeclimate bot Jun 15, 2020
Repository owner deleted a comment from codeclimate bot Jun 15, 2020
Repository owner deleted a comment from codeclimate bot Jun 15, 2020
Repository owner deleted a comment from codeclimate bot Jun 15, 2020
@picandocodigo picandocodigo marked this pull request as draft June 15, 2020 22:30
/*
* Create date_query args according to https://codex.wordpress.org/Class_Reference/WP_Query#Date_Parameters
*/
public function create_date_query_args($args, $params) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function create_date_query_args has a Cognitive Complexity of 18 (exceeds 5 allowed). Consider refactoring.

/*
* Create date_query args according to https://codex.wordpress.org/Class_Reference/WP_Query#Date_Parameters
*/
public function create_date_query_args($args, $params) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function create_date_query_args has a Cognitive Complexity of 14 (exceeds 5 allowed). Consider refactoring.

@klemens-st
Copy link
Collaborator

I've realised I have a few more branches with interesting improvements but some of them depend on #288 so will need to merge that first.

@picandocodigo picandocodigo force-pushed the refactor branch 3 times, most recently from 07ee67e to 48f1414 Compare June 18, 2020 21:22
@picandocodigo picandocodigo marked this pull request as ready for review June 24, 2020 22:45
Comment on lines -49 to +37
public function with_id($id){
if (preg_match('/\+/', $id)){
if ( preg_match('/(-[0-9]+)+/', $id, $matches) ){
public function with_id($cat_id){
if (preg_match('/\+/', $cat_id)){
if ( preg_match('/(-[0-9]+)+/', $cat_id, $matches) ){
$this->exclude = implode(',', explode("-", ltrim($matches[0], '-') ));
}
$lcp_category_id = array_map( 'intval', explode( "+", $id ) );
} else {
$lcp_category_id = $id;
return array_map( 'intval', explode( "+", $cat_id ) );
}
return $lcp_category_id;
return $cat_id;
Copy link
Collaborator

@klemens-st klemens-st Jun 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole method is bugged, doing id="1+2-3" doesn't work (nothing to do with your refactoring, it's been so for a long time). I'm preparing a PR to merge with this branch so that category things should be good and done.

@klemens-st
Copy link
Collaborator

@picandocodigo I've found something by accident while trying to help someone.

The issue she's having does not exist on the master branch, but when I first accidentally tried it on the 'refactor' branch it did exist. Maybe something to do with date refactoring?

@picandocodigo
Copy link
Owner Author

@zymeth25 that's weird. Yeah I'm testing some stuff on this branch now, and orderby is not working either. I'm going to test this and the issue with after and add the fixes to this branch.

@picandocodigo
Copy link
Owner Author

orderby is workig fine, it's just order_by is being issued on the issue 😬
Continuing with after...

@klemens-st
Copy link
Collaborator

Yeah I know she made a mistake with order_by, but I only tested after and it didn't work on this branch for some reason

@codeclimate
Copy link

codeclimate bot commented Jun 27, 2020

Code Climate has analyzed commit 5e286af and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@picandocodigo
Copy link
Owner Author

@zymeth25 I think this was the issue 5e286af, at least working for me locally now.

@klemens-st
Copy link
Collaborator

@picandocodigo Yeah, works for me too.

@picandocodigo
Copy link
Owner Author

Thinking we should merge this PR and your 2 PRs and publish a new version on Monday 🤔

@klemens-st
Copy link
Collaborator

Yeah good idea. That's enough for a release, we should make sure all of it works in all the crazy user scenarios we don't know about 😄

@picandocodigo picandocodigo merged commit b093901 into master Jun 27, 2020
@picandocodigo picandocodigo deleted the refactor branch June 27, 2020 21:35
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.

2 participants