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

Add ParseTreeProperty class #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

HermanPeeren
Copy link

fixes #7

* added void return type to put()
* deleted code-tag in description in docblock
@marcospassos
Copy link
Collaborator

@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?

{
$value = $this->get($node);
$this->storage->detach($node);
return $value;
Copy link
Collaborator

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

*/
public function removeFrom(ParseTree $node)
{
$value = $this->get($node);
Copy link
Collaborator

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

public function get(ParseTree $node)
{
$value = null;
if ($this->storage->contains($node)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

blank line before

if ($this->storage->contains($node)) {
$value = $this->storage->offsetGet($node);
}
return $value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

blank line before

*/
class ParseTreeProperty
{
protected $storage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not private?

Copy link
Author

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.

@HermanPeeren
Copy link
Author

OK, thanks!

  • Was also working on unit-tests for the php-runtime, as they are missing in the repo now. Or do you already have some tests, that are not in this repo yet?
  • You'll probably comply to PSR-12, or do you have a specific codestyle-document for the php-runtime-files? Just adjusted mine.

@marcospassos
Copy link
Collaborator

@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.

@HermanPeeren
Copy link
Author

@HermanPeeren we follow the style specified here.

Haha, I cou;ld have seen that... have to look better. Will use that in PHPstorm.

Thanks for reviewing, @marcospassos ! Very welcome!

@marcospassos
Copy link
Collaborator

Hi @HermanPeeren! Are you still interested in working on this PR?

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.

Class ParseTreeProperty
2 participants