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

Scope improve usability - addCondition() should not wrap #664

Merged
merged 9 commits into from
Jul 24, 2020

Conversation

mvorisek
Copy link
Member

@mvorisek mvorisek commented Jul 23, 2020

makes even more sense with #663 as we use term "condition" for simple condition only

when you, at least I, add a condition, I want to add a simple one

@mvorisek mvorisek force-pushed the scope_improve_add_simple_cond branch from 0ea6951 to 7414cef Compare July 23, 2020 22:06
@mvorisek mvorisek changed the title Scope improve add simple cond Scope improve usability - addCondition adds a simple condition Jul 23, 2020
@mvorisek mvorisek changed the title Scope improve usability - addCondition adds a simple condition Scope improve usability - addCondition() should not wrap Jul 23, 2020
{
$this->add(new self([func_get_args()]));
Copy link
Collaborator

@georgehristov georgehristov Jul 24, 2020

Choose a reason for hiding this comment

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

I would only change the statement to
$this->add(static::createAnd(func_get_args()));

Why would you want to remove the flexibility? It does create what you expect + more and consistent with Model::addCondition. And code is also simpler and all handling is in one place (__constructor).

We can even use same method in Model::addCondition
$this->scope()->addCondition(...func_get_args());

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would you want to remove the flexibility? It does create what you expect + more and consistent with Model::addCondition. And code is also simpler and all handling is in one place (__constructor).

We can even use same method in Model::addCondition
$this->scope()->addCondition(...func_get_args());

I like to think in consistent design.

There are two things:

a) I do not think it is good to wrap when we add a simple condition. The structure should really look like to what you add, if you add name = "George", then I would never expect a Compound condition from it. And, this was also not the case before (in case of array structure). We can agree on this, right?

b) Allow to pass a conditions as an array, ie. addCondition([[1. cond], [2. cond]]), that is your concern, right?

If the input is an array, we can use CompoundCondition - but what is the usecase vs. not using createAnd/createOr which I think is easier to read, because:

  • of explicit And/Or in text
  • you can still add CompoundCondition object using this method
  • we enforce the user to use object as soon as possible, ie. to not create an array first

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have several concerns:

  • Model will use addCondition in one way (BC considerations) and CompoundCondition in another (inconsistent)
  • handling of creation of BasicCondition will be distributed on several places instead of only one (in constructor). When you addCondition you expect a condition to be added to the scope. And it is. in what way is internal workings.
  • we reduce the flexibility
  • we spend way too much time on theoretical discussions. At some point benefits of collaborative creation are wiped out as we cannot go further and just go in circles

@mvorisek mvorisek force-pushed the scope_improve_add_simple_cond branch 2 times, most recently from df4e40a to bd23552 Compare July 24, 2020 09:17
@mvorisek mvorisek force-pushed the scope_improve_add_simple_cond branch from bd23552 to 38e853c Compare July 24, 2020 09:30
if ($nestedCondition instanceof AbstractCondition) {
$condition = $nestedCondition;
} else {
if (!is_array($nestedCondition)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe use one-liner casting
$nestedCondition = (array) $nestedCondition;

Copy link
Member Author

@mvorisek mvorisek Jul 24, 2020

Choose a reason for hiding this comment

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

no! we assigning the same is a bad design, really - at least much easier to read - if something, we do something, otherwise we do nothing

Copy link
Collaborator

@georgehristov georgehristov left a comment

Choose a reason for hiding this comment

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

👍

@georgehristov georgehristov merged commit 120cd8a into develop Jul 24, 2020
@georgehristov georgehristov deleted the scope_improve_add_simple_cond branch July 24, 2020 11:07
if (func_num_args() === 1 && $field instanceof AbstractCondition) {
$condition = $field;
} elseif (func_num_args() === 1 && is_array($field)) {
$condition = static::createAnd(func_get_args());
Copy link
Member Author

Choose a reason for hiding this comment

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

this was wrong, fixed in #1125

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

Successfully merging this pull request may close these issues.

2 participants