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

Selection: moved caching related functionality to separate classes #185

Merged
merged 1 commit into from Oct 11, 2017

Conversation

foxycode
Copy link
Contributor

@foxycode foxycode commented Oct 9, 2017

  • bug fix? no
  • new feature? yes
  • BC break? no

@dg Cache implementation is a long term problem in NDBT. While this is definitely not perfect, it's a starting point from which I would like to continue making NDBT code more clear, which wil help in future development. I spent two days with this, so I hope you appreciate it. Also, if you agree, I can maintain this repo as we spoken on NettCamp. There is lot for review and merge.

@dg
Copy link
Member

@dg dg commented Oct 9, 2017

Thanks for pull request. If I understand it, this is a pure refactoring that separates the code into new classes but does not change the functionality?

@foxycode
Copy link
Contributor Author

@foxycode foxycode commented Oct 9, 2017

@dg Yes, exactly :)

}


public function setGeneralCacheKey(?string $key)
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

Because it is targeted to 7.1, you can use return type void.



/**
* @param mixed $accessedColumns
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

Please don't use property name in @param. I know that PhpStorm appends it automatically, but better is to be consistent with rest of code.

Btw is it really mixed or array?

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

It is really mixed


/**
* Loads cache of previous accessed columns and returns it.
* @return array|bool
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

When it returns bool?

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

This is just move from original class. I wasn't sure so I reather left it as is.

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

There are more variables that use null and bool to distinguish some states. I would like to fix this in second run.

Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

It seems that array_keys() can return only array, or not?

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

Yes, and it passes tests, fixed.



/**
* @param mixed $observeCache
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

mixed or bool?

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

Here iare stored Selection instances.

@foxycode foxycode force-pushed the cache branch 2 times, most recently from 0b2d5f3 to 75b5c5c Compare Oct 9, 2017
@foxycode
Copy link
Contributor Author

@foxycode foxycode commented Oct 9, 2017

Rest fixed



/**
* @param mixed
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

$previousAccessedColumns has typehint array and here is mixed

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

My bad. I copied properties as they were. Fixed on all properties.

/**
* @param mixed
*/
public function setAccessedColumns($accessedColumns): void
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

$accessedColumns has typehint array and here is mixed

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

Fixed.



/**
* @return mixed
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

$accessedColumns has typehint array and here is mixed

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

Fixed



/**
* @param mixed
Copy link
Member

@dg dg Oct 9, 2017

Choose a reason for hiding this comment

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

$observeCache has typehint bool and here is mixed

Copy link
Contributor Author

@foxycode foxycode Oct 9, 2017

Choose a reason for hiding this comment

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

Fixed, allowed only Selection.

@foxycode foxycode force-pushed the cache branch 2 times, most recently from 89441c5 to a22f121 Compare Oct 9, 2017
@foxycode
Copy link
Contributor Author

@foxycode foxycode commented Oct 10, 2017

@dg Anything else I can do so you merge it?

/** @var string */
protected $specificCacheKey;

/** @var mixed of touched columns */
Copy link
Member

@dg dg Oct 10, 2017

Choose a reason for hiding this comment

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

What means mixed of? Is it array or not?

Copy link
Contributor Author

@foxycode foxycode Oct 10, 2017

Choose a reason for hiding this comment

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

There was lot of mess arround mixed properties, so I get rid of them and simplify code in separate commit. Let me know what you think.



/**
* @return mixed
Copy link
Member

@dg dg Oct 11, 2017

Choose a reason for hiding this comment

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

It seems according to the code that it is array|null, or not? For getReferenced too.

Copy link
Contributor Author

@foxycode foxycode Oct 11, 2017

Choose a reason for hiding this comment

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

Yes, you are right


public function setAccessedColumn(string $key, bool $value): void
{
if (!$this->cache) {
Copy link
Member

@dg dg Oct 11, 2017

Choose a reason for hiding this comment

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

Just such a stupid thing, maybe positive condition can be used here and in setAccessedColumns

if ($this->cache) {
     $this->accessedColumns[$key] = $value;
}
```

Copy link
Contributor Author

@foxycode foxycode Oct 11, 2017

Choose a reason for hiding this comment

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

Fixed on both places.

@dg
Copy link
Member

@dg dg commented Oct 11, 2017

Now it look much better, thanks!

/**
* @return array|null
*/
public function &getReferencing(string $generalCacheKey)
Copy link
Member

@dg dg Oct 11, 2017

Choose a reason for hiding this comment

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

Sry, now I realized that it can be written native as &getReferencing(string $generalCacheKey): ?array

Copy link
Contributor Author

@foxycode foxycode Oct 11, 2017

Choose a reason for hiding this comment

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

Yes, I should realize that too. Fixed

@dg
Copy link
Member

@dg dg commented Oct 11, 2017

Do you want to have two commits, one with mixed and the other without mixed properties?

@foxycode
Copy link
Contributor Author

@foxycode foxycode commented Oct 11, 2017

@dg No, it was just for easier code review, squashed.

It would be good to resolve other PR's before I get further. Like this one, which is not needed now: #161 Will you do that or should I take care of that for you?

@dg dg merged commit a561254 into nette:master Oct 11, 2017
2 of 3 checks passed
@dg
Copy link
Member

@dg dg commented Oct 11, 2017

Thanks, great!

dg added a commit that referenced this issue Apr 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants