-
Notifications
You must be signed in to change notification settings - Fork 78
Conversation
* @param string $variableName | ||
* @return self | ||
*/ | ||
public function setVariableName($variableName) |
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 remove this setter, unless strictly required. Making it private
is also OK
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.
Variable name is allowed to be null in constructor, so there should be method to set it later. Other tags like @param
or @author
are working same way.
} | ||
|
||
/** | ||
* @return string |
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.
May be null
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.
done
/** | ||
* @var string | ||
*/ | ||
protected $variableName = null; |
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.
= null
redundant
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.
done
class VarTag extends AbstractTypeableTag implements TagInterface | ||
{ | ||
/** | ||
* @var string |
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.
May be null
, so should be string|null
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.
done
} | ||
|
||
/** | ||
* @return string |
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.
{@inheritDoc}
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.
done
} | ||
|
||
/** | ||
* @return string |
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.
{@inheritDoc}
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.
done
*/ | ||
public function generate() | ||
{ | ||
$output = '@var' |
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.
Return directly
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.
done
* @param array $types | ||
* @param string $description | ||
*/ | ||
public function __construct($variableName = null, $types = [], $description = null) |
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.
is $types
a TypeGenerator[]
here? Can we enforce that?
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.
No, it is string or array of strings, passed to\Zend\Code\Generator\DocBlock\Tag\AbstractTypeableTag::setTypes
public function initialize($tagDocblockLine) | ||
{ | ||
$match = []; | ||
if (!preg_match('#^(.+)?(\$[\S]+)[\s]*(.*)$#m', $tagDocblockLine, $match)) { |
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 [\s]*
can be simplified to \s*
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.
done
@Ocramius, updated |
@red-led this is ready to go. I will still remove any premature mutability (setters) on merge, plus I'll need to rebase, since patch needs to be applied to |
…leTag` which, supports `string|string[]`
… to prevent misuse, as it is needed for the `TagManager` to work correctly
Pretty common tag for class variables.