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

Adding PHPDocs to templates #284

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@wow-apps
Copy link

wow-apps commented Oct 4, 2018

PR Changelog

99% of changes are adding PHPDocs

EmptyAuthenticator.tpl.php

src/Resources/skeleton/authenticator/EmptyAuthenticator.tpl.php

  • Added PHPDocs for class and methods
  • Added method createAuthenticatedToken from Symfony\Component\Security\Guard\AuthenticatorInterface

EmptySecurityController.tpl.php

src/Resources/skeleton/authenticator/EmptySecurityController.tpl.php

  • Added PHPDoc for class

LoginFormAuthenticator.tpl.php

src/Resources/skeleton/authenticator/LoginFormAuthenticator.tpl.php

  • Added PHPDocs for class and methods
  • Removed use TargetPathTrait; - it's already used in extended class

Command.tpl.php

src/Resources/skeleton/command/Command.tpl.php

  • Added PHPDocs for class, methods and variable

Controller.tpl.php

src/Resources/skeleton/controller/Controller.tpl.php

  • Added PHPDocs for class and method

Controller.tpl.php

src/Resources/skeleton/crud/controller/Controller.tpl.php

  • Added PHPDocs for class and method

Entity.tpl.php

src/Resources/skeleton/doctrine/Entity.tpl.php

  • Added PHPDocs for class, variable and method

Fixtures.tpl.php

src/Resources/skeleton/doctrine/Fixtures.tpl.php

  • Added PHPDocs for class and method

Repository.tpl.php

src/Resources/skeleton/doctrine/Repository.tpl.php

  • Added PHPDocs for class and methods

Subscriber.tpl.php

src/Resources/skeleton/event/Subscriber.tpl.php

  • Added PHPDocs for class and methods

Type.tpl.php

src/Resources/skeleton/form/Type.tpl.php

  • Added PHPDocs for class and methods

UserProvider.tpl.php

src/Resources/skeleton/security/UserProvider.tpl.php

  • Added PHPDoc for class

Voter.tpl.php

src/Resources/skeleton/security/Voter.tpl.php

  • Added PHPDocs for class and methods

Encoder.tpl.php

src/Resources/skeleton/serializer/Encoder.tpl.php

  • Added PHPDocs for class and methods

Functional.tpl.php

src/Resources/skeleton/test/Functional.tpl.php

  • Added PHPDoc for class

Unit.tpl.php

src/Resources/skeleton/test/Unit.tpl.php

  • Added PHPDoc for class

Extension.tpl.php

src/Resources/skeleton/twig/Extension.tpl.php

  • Added PHPDocs for class and methods

Constraint.tpl.php

src/Resources/skeleton/validator/Constraint.tpl.php

  • Added PHPDocs for class and methods

Validator.tpl.php

src/Resources/skeleton/validator/Validator.tpl.php

  • Added PHPDocs for class and methods

Class.tpl.php

src/Resources/skeleton/Class.tpl.php

  • Added PHPDoc for class
@weaverryan
Copy link
Member

weaverryan left a comment

Hi there!

Thanks for this! I’ve started reviewing this. The big change that’s needed is that a lot of the documentation doesn’t add value. I’m +1 for adding docs that describes something that is not already obvious.

Could you remove the docs that are already obvious from existing type-hints, etc?

Thanks!

@wow-apps

This comment has been minimized.

Copy link

wow-apps commented Oct 17, 2018

Hi @weaverryan . Sorry, I was on PHPDD conference in Dresden and have no time to finish this PR.
Yes, sure, I'll take attention on your comments and finish this in a few days.

see you again on SymfonyCon this year ;)

wow-apps added some commits Oct 21, 2018

@@ -26,7 +23,7 @@ class <?= $class_name."\n" ?>
/**
* @return int|null
*/
public function getId(): ?int

This comment has been minimized.

@wow-apps

wow-apps Oct 21, 2018

It's php 7.0.8 defined in composer.json
But return type or null come just in php 7.1

This comment has been minimized.

@ckrack

ckrack Nov 1, 2018

Contributor

make:entity requires PHP 7.1

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

That's correct. Let's revert this change.

@wow-apps

This comment has been minimized.

Copy link

wow-apps commented Oct 21, 2018

Hey @weaverryan

I've refactor code, please make a code review before I'll fix tests (some tests depends on created templates)

Few words about last commit:

  • Yes, we can add return type hinting if it not defined in Interface. I've add everywhere I can;
  • We can't add type hinting for arguments if they are not defined in Interface;
  • I've replace \n everywhere in templates on PHP_EOL for support new line by not UNIX hosts
  • I've remove in one or two places return type hinting something | null - for example: 9d93c0e#r226875323 because this feature comes with php 7.1, but in composer.json for current bundle php version required is 7.0.8
  • PHPDocs for all extended function were replaced on {@inheritdoc}
  • @package and class name removed

have a good time while reviewing ;)

@javiereguiluz
Copy link
Member

javiereguiluz left a comment

I left some comments about additions which are not necessary because IDEs can get the information from other ways. Thanks!

}

/**
* {@inheritdoc}

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

I dislike these {@inheritdoc} so much 😢 Isn't PhpStorm smart enough to do that without adding this?

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

I would be nice indeed (I don't use PhpStorm, and I don't care about theses inheritdoc)

use TargetPathTrait;

<?= $user_is_entity ? " private \$entityManager;\n" : null ?>
<?= $user_is_entity ? " /** @var EntityManagerInterface */" . PHP_EOL . " private \$entityManager;" . PHP_EOL . PHP_EOL : null ?>

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

The @var annotations in these properties and the __cosntruct() method's @param are not needed because constructor arguments are type-hinted and IDEs can infer the types.

protected function execute(InputInterface $input, OutputInterface $output)
{
$io = new SymfonyStyle($input, $output);
$symfonyStyle = new SymfonyStyle($input, $output);

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

I'm not sure I like this variable name. First, we use $io everywhere else in Symfony. Second, this object is not really a "Symfony style" but an "input + output object that displays things using the Symfony style".

{
/**
* <?= ucwords($route_name) ?> index page

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

Please let's remove this generic comment because it doesn't add much value.

{
/**
* <?= ucwords($route_name) ?> index page

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

In this method we can remove the generic comment, the @param and the @return because IDEs can infer all that with the arguments type hints and the return types.

Same in the other methods of this class.

@@ -26,7 +23,7 @@ class <?= $class_name."\n" ?>
/**
* @return int|null
*/
public function getId(): ?int

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

That's correct. Let's revert this change.

* @method <?= $entity_class_name; ?>|null find($id, $lockMode = null, $lockVersion = null)
* @method <?= $entity_class_name; ?>|null findOneBy(array $criteria, array $orderBy = null)
* @method <?= $entity_class_name; ?>[] findAll()
* @method <?= $entity_class_name; ?>[] findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
*/
class <?= $class_name; ?> extends ServiceEntityRepository
{
/**

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

We can remove this entire PHPdoc block for the same reasons given for the other blocks.


use Symfony\Component\Validator\Constraint;

/**
* Class <?= $class_name . PHP_EOL ?>

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 3, 2019

Member

Let's remove this generic comment.

@lyrixx
Copy link
Member

lyrixx left a comment

Many PHP Doc could be removed

@@ -1,7 +1,7 @@
<?= "<?php\n" ?>
<?= "<?php" . PHP_EOL ?>

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

This should be reverted. We use \n everywhere, every-time

}

/**
* {@inheritdoc}

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

I would be nice indeed (I don't use PhpStorm, and I don't care about theses inheritdoc)

<?= $user_needs_encoder ? " /** @var UserPasswordEncoderInterface */" . PHP_EOL . " private \$passwordEncoder;" . PHP_EOL : null ?>

/**
* <?= $class_name; ?> constructor.

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

This should be deleted. it Does not not bind new information.

*
<?= $user_is_entity ? " * @param EntityManagerInterface \$entityManager" . PHP_EOL : null ?>
* @param RouterInterface $router
* @param CsrfTokenManagerInterface $csrfTokenManager

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

This should be deleted. it Does not not bind new information.


namespace <?= $namespace ?>;

use <?= $entity_full_class_name ?>;
use <?= $form_full_class_name ?>;
<?php if (isset($repository_full_class_name)): ?>
<?php if (!empty($repository_full_class_name)): ?>

This comment has been minimized.

@lyrixx

lyrixx Jan 4, 2019

Member

Why this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment