Refactor to add AttributableOp interface for ops that have attribute groups #108
Conversation
eric-therond
left a comment
There was a problem hiding this comment.
I like the idea,
It looks good, I left two comments.
lib/PHPCfg/Printer.php
Outdated
|
|
||
| if ($op instanceof Op\Stmt\Function_ || $op instanceof Op\Stmt\Class_ || $op instanceof Op\Stmt\Property || $op instanceof Op\Expr\Param) { | ||
| if ($op instanceof Op\AttributableOp) { | ||
| $result .= $this->renderAttrGroups($op->attrGroups); |
There was a problem hiding this comment.
| $result .= $this->renderAttrGroups($op->attrGroups); | |
| $result .= $this->renderAttrGroups($op->getAttributeGroups()); |
and then the attrGroups property can be made private?
There was a problem hiding this comment.
Refactored a bit more, would love some more eyes
eric-therond
left a comment
There was a problem hiding this comment.
Looks good.
This is the first time I've seen traits in action, I don't have a lot to say but it helps a lot to factorize code.
| $this->byRef = $byRef; | ||
| $this->variadic = $variadic; | ||
| $this->attrGroups = $attrGroups; | ||
| $this->setAttributeGroups(...$attrGroups); |
There was a problem hiding this comment.
Why using the ... operator? $attrGroups is defined as an array on the __construct param.
There was a problem hiding this comment.
To provide type safety. setAttributeGroups is defined as setAttributeGroups(AttributeGroup ... $attrGroups)... There are def other ways, but that was the intent here.
Remove unused imports Co-authored-by: eric-therond <[email protected]>
|
Merging |
Proof of concept, feedback requested. /cc @eric-therond