Some suggestions

[chasepeeler]

Just used your library to generate some of my doctrine entities. Overall I was very pleased with how things worked. A few suggestions though:

1.) Need a way to insert a blank line in a docblock. Some of my docblocks are really cluttered since there isn't any spacing between various sections.

2.) Took me a while to figure out that the setType method for PhpMethod was the return type. Better documentation or better naming (like setReturnType) would be work.

3.) Need a way to disable non-scalar type hints as well. The work around was to not declare a type for the parameters, and then add a param tag to the docblock.

4.) Not sure why, but, phpstorm thinks the getDocBlock methods are protected, and complains.

5.) Since you have concrete classes for different docblock tags (ParamTag, PropertyTag, etc), you should tailor their create methods to the standards for those types. For example, the ParamTag::create would take in a variable name, and, optionally, a type. Otherwise, there really isn't an advantage to using those instead of the TagFactory.

[chasepeeler]

I'm more than happy to split any of the above into their own tickets - but I wanted to get your thoughts on them before I did that. Otherwise I might be spamming you with a bunch of tickets which you don't actually want.

[gossi]

Hey,

thanks for your suggestions:

1. I probably missunderstand your use-case wrong, though I get it is valid ;)

2. Yep, I discussed this a long time with my self to align on setType(), as it is a consistent method across all things you can type. A possible setReturnType() would be an outlier although it is something you possible look for at first. Rule of thumb: Whenever something can take a type use setType() - would improve the docs, for sure 👍

3. In which locations you want to do this? Globally? Maybe the TypePart can handle this, together with docblock?

4. 🤷‍♂️

5. That would be a good use-case for the spread operator - which just wasn't available the time I developed these libraries.

Quite a lot of them are actually towards the docblock repo, but doesn't matter, they are connected ... somehow. PRs are welcome for each 🤗

[chasepeeler]

1.)
My entity classes have their properties as protected items, which are exposed via __get (which does some other stuff under the hood, which is why they aren't just public). So, I have @Property tags in the class docblock. This is what one looks like

/**
 * Class MilestoneTypeHistory
 *
 * @package ESP\Model\Data
 *
 * @ORM\Entity
 * @ORM\Table(name="milestone_types_history")
 *
 * @property int $id
 * @property User $lastUpdatedBy
 * @property MilestoneType $milestoneType
 * @property \DateTime $lastUpdatedOn
 * @property string $directions
 * @property bool $active
 * @property string $name
 * @property string $description
 * @property int $expectedDuration
 * @property MilestoneTypeInstallGuide $milestoneTypeInstallGuide
 */

To generate the above exactly like you see it with your library, I'd have to build the docblock as a string, and then set it. If I try and build it, it'll look like this:

/**
 * Class MilestoneTypeHistory
 * @package ESP\Model\Data
 * @ORM\Entity
 * @ORM\Table(name="milestone_types_history")
 * @property int $id
 * @property User $lastUpdatedBy
 * @property MilestoneType $milestoneType
 * @property \DateTime $lastUpdatedOn
 * @property string $directions
 * @property bool $active
 * @property string $name
 * @property string $description
 * @property int $expectedDuration
 * @property MilestoneTypeInstallGuide $milestoneTypeInstallGuide
 */

The lack of blank lines just make it more difficult to read. It's not a huge deal. I still choose to build out the docblock, giving up the blank lines. I can always add them back manually. Just a little thing that would be nice to be able to do. I can see someone that is utilizing a lot of different tags also wanting to add some separation between them.

Let me know, and I'll create a new ticket in your docblock repo for this.

3.) At the very least, a global setting like you have for scalar type hints and return types. I think that would match up with current functionality. Extending the current functionality for methods to allow specifying scalar type hints, nonscalar type hints, and return type hints per method would be nice, but, I don't know how much demand there would be for that level of control. If you were going to go that route, you probably should make the type hints configurable per parameter. I find that about half the time I don't want type hints, so I just make a stylistic choice to omit them altogether (except for a few rare circumstances). Someone else might choose to include them whenever they can - so the ability to turn it on/off per parameter would be useful then.

Would you like me to create a separate ticket for this?

4.) PhpStorm is complaining that getDockblock() is protected for PhpMethod and PhpProperty. For PhpMethod, it resolves back to the abstract protected method in ParametersPart, and for PhpProperty, it resolves back to the abstract protected method in TypeDocblockGeneratorPart. However, both PhpMethod and PhpProperty extend AbstractPhpMember, which uses DocblockPart, and has a non-abstract public getDocblock.

https://3v4l.org/O4Yp6 basically shows what is happening. Honestly, I'm not convinced that isn't a PHP bug. It seems like it should either give you an error based on method name conflicts between two traits (per http://php.net/manual/en/language.oop5.traits.php#language.oop5.traits.conflict) or an error related to different visibilities between the parent and child method.

I can't make any promises on pull requests. Pretty busy with my main job (I actually started typing this up yesterday and didn't get to finish it) but I'll do what I can.

[gossi]

1. Ok, understand it now. The way you do it, is the way it should be done to bring the comments there. In order to format the docblock, please create an issue over at the repo

2. Sounds like an interessting topic. Please create its own ticket, provide the background, discuss both solutions you have in mind (maybe they are combinable?) and then find a solution in that issue we can implement

3. I see, I put these methods in the trait, so eclipse isn't moaning about them beeing missing ;) I set my environment to php 5.* - So, maybe PhpStorm uses a php7 parser and this is where different situations arise. In general, upgrade this library to php7, drop support for php5 is the way forward, which would also tackle Using nikic/php-parser version 4 cause errors #55

After all, good stuff you put together - very welcoming. Since all our time is limited (and I do not even wrote code in php since two years but maintaining stuff), let's craft good specs together so ideas are not lost and find its way into code.