-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add ParseTreeProperty class #8
base: master
Are you sure you want to change the base?
Conversation
* added void return type to put() * deleted code-tag in description in docblock
@HermanPeeren I'll set up a workflow for running automated tests and code style checks over the next days, and then I'm going to come back to this, ok? |
src/Tree/ParseTreeProperty.php
Outdated
{ | ||
$value = $this->get($node); | ||
$this->storage->detach($node); | ||
return $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put a line before the return statement
src/Tree/ParseTreeProperty.php
Outdated
*/ | ||
public function removeFrom(ParseTree $node) | ||
{ | ||
$value = $this->get($node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation should be 4 chars long
src/Tree/ParseTreeProperty.php
Outdated
public function get(ParseTree $node) | ||
{ | ||
$value = null; | ||
if ($this->storage->contains($node)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line before
src/Tree/ParseTreeProperty.php
Outdated
if ($this->storage->contains($node)) { | ||
$value = $this->storage->offsetGet($node); | ||
} | ||
return $value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line before
src/Tree/ParseTreeProperty.php
Outdated
*/ | ||
class ParseTreeProperty | ||
{ | ||
protected $storage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. It was private first, but I changed it to protected to be abele to use it in the drived class... which is not a good idea. Change it back. Thanks.
OK, thanks!
|
…ult from code review by @marcospassos.
@HermanPeeren we follow the style specified here. Currently, the library is covered by functional tests. Unit tests and performance tweaks are great contributions at the moment. |
Haha, I cou;ld have seen that... have to look better. Will use that in PHPstorm. Thanks for reviewing, @marcospassos ! Very welcome! |
Hi @HermanPeeren! Are you still interested in working on this PR? |
fixes #7