Missing DocBlocks, would a PR be accepted?

[Jacco-V]

Hi,

I noticed that nearly none of the class methods in the various classes in the Transphporm src directory have a DocBlock (a quick count comes up with 33 DocBlocks for a total of 229 class methods, 41 classes and 5 interfaces).

In my opinion, those missing DocBlocks should be added : )

However, I'm aware that not everybody likes them. Some think them pointless, others don't like the added lines, feel it adds clutter, you name it.

I'm willing to add all those missing DocBlocks and create a PR, but given that it would cost a bit of time, I thought it would be better to chime in here before I add them, just to see the PR rejected : )

So, how do you feel about DocBlocks? Would they be appreciated? Or would you rather not have them?

[TRPB]

If they add information that isn't already available in the method header then yes they're useful. If they just duplicate what's already there, then there's no point in them.

/*
@method foo
@arg $bar
@arg $baz
*/
public function foo($bar, $baz)

Totally pointless. Any docblock that can be generated from the source is 100% wasted bytes.

[Jacco-V]

A good DocBlock adds information that cannot be generated from the source code. A good DocBlock makes sure you don't need to read the entire function, but instead know what the function does, what arguments (and their type) are required and what can be expected to be returned, if anything.

Proper DocBlocks are the full documentation for a given class or interface. They are helpful for spotting errors during a code review. In case you are writing unit tests 'after the fact', your DocBlock provides a template for the functionality that you want to cover with your tests. If you write your code test-driven, the DocBlocks description follows as a logical result from the functionality tested in the test cases.

Here is a copy of Rule.php with DocBlock:

<?php
/**
 * @description     Transformation Style Sheets - Revolutionising PHP templating    *
 * @author          Tom Butler tom@r.je                                             *
 * @copyright       2015 Tom Butler <tom@r.je> | https://r.je/                      *
 * @license         http://www.opensource.org/licenses/bsd-license.php  BSD License *
 * @version         1.0                                                             *
 */

namespace Transphporm;

/**
 * [???? Some sort of rule (base) class that can/will be run with certain timeouts ???]
 */
class Rule {

    /**
     * One second.
     */
    const S = 1;

    /**
     * One minute in seconds.
     */
    const M = 60;

    /**
     * One hour in seconds.
     */
    const H = 3600;

    /**
     * One day in seconds.
     */
    const D = 86400;

    /**
     * @var string $query ???
     */
    private $query;

    /**
     * @var [??? \Transphporm\Parser\Tokens ???] $pseudo ???
     */
    private $pseudo;

    /**
     * @var int $depth ???
     */
    private $depth;

    /**
     * @var integer $index ???
     */
    private $index;

    /**
     * @var string $file ???
     */
    private $file;

    /**
     * @var array $properties ???
     */
    private $properties = [];

    /**
     * @var int $lastRun A timestamp keeping track of when self::touch() was last called.
     */
    private $lastRun = 0;

    /**
     * Constructor.
     *
     * @param string $query ???
     * @param [??? \Transphporm\Parser\Tokens ???] $pseudo ???
     * @param int $depth ???
     * @param int $index ???
     * @param string $file ???
     * @param int $line ???
     * @param array $properties ???
     */
    public function __construct($query, $pseudo, $depth, $index, $file, $line, array $properties = []) {
        $this->query = $query;
        $this->pseudo = $pseudo;
        $this->depth = $depth;
        $this->index = $index;
        $this->file = $file;
        $this->line = $line;
        $this->properties = $properties;
    }

    /**
     * Get any defined class property.
     *
     * @param string $name The name of the property to get. An E_NOTICE is emitted when attempting to get a non-defined member property.
     *
     * @return mixed The value of the member property, null when the member property does not exist.
     */
    public function __get($name) {
        return $this->$name;
    }

    /**
     * Set any defined class property.
     *
     * @param string $name The name of the member property to set.
     * @param mixed $value The value to assign to the member property name @param $name.
     */
    public function __set($name, $value) {
        $this->$name = $value;
    }

    /**
     * Set the last-run timestamp to now.
     */
    public function touch() {
        $this->lastRun = time();
    }

    /**
     * Figure out if enough time has passed since our last run, to require a new run.
     *
     * @param string $frequency A string in the format of "n S|M|H|D" where n is an integer and S, M, H and D indicate Seconds, Minutes, Hours and Days respectively.
     * @param int|null $time An optional timestamp to match the timeout conditions against. If ommited or null, the current timestamp will be used.
     *
     * @return bool True when a new run is required, false otherwise.
     */
    private function timeFrequency($frequency, $time = null) {
        if ($time === null) $time = time();
        $num = (int) $frequency;
        $unit = strtoupper(trim(str_replace($num, '', $frequency)));

        $offset = $num * constant(self::class . '::' . $unit);

        if ($time > $this->lastRun + $offset) return true;
        else return false;
    }

    /**
     * Check if the rule should be run, based upon [???? various unclear conditions ????].
     *
     * @param int|null $time An optional timestamp to match the timeout conditions against. If ommited or null, the current timestamp will be used.
     *
     * @return bool True if the rules should be run, false otherwise.
     */
    public function shouldRun($time = null) {
        if (isset($this->properties['update-frequency']) && $this->lastRun !== 0) {
            $frequency = $this->properties['update-frequency']->read();
            $static = ['always' => true, 'never' => false];
            if (isset($static[$frequency])) return $static[$frequency];
            else return $this->timeFrequency($frequency, $time);
        }
        else return true;
    }
}

As you can see, there are quite some unknowns. I could probably fill in the missing pieces by digging around in the code, but it takes digging. If the DocBlocks were complete, the project would become much more accessible for outside collaborators.

Note that, for this example, I've chose a very verbose version and I did not break lines at a certain max character width. By eliminating the empty lines in the DocBlocks, it would look much more compact.

[Jacco-V]

Would you accept a Pull Request with DocBlock as shown above?

[TRPB]

Yes, any docblock that actually includes useful descriptions will be merged