Specify hooks formatting

[Crell]

So we have something concrete to chew on.

Having used hooks on a project for the last few months, this feels about right to me. I could see arguments for other restrictions on when to use one-big-line hooks, but they do have their place. (Passthroughs, for instance.)

Resolves #88

[TimWolla]

Apart from the inline remarks, as I reader I would expect the “expanded” syntax to be defined first: That's the only syntax that works in every case. The short syntax then is a legal exception that MAY be used.

Currently it reads quite awkward, because the definition starts with the exception “If […]”, without properly explaining the “else” part.

---

Also as mentioned in the issue, I disagree with the brace placement because of the inconsistency with methods. The second example given by @edorian in #88 (comment) looks like the best compromise to me.

[Crell]

• Rewrote the hooks section to go from longer form to shorter form.
• Used more bullet points.
• Moved the hooks section later.
• Added a section on interface properties, too.

Thoughts now?

[Crell]

As for the brace spacing (I deliberately left that for the moment), here's some examples from a project I'm working on now in 8.4 that makes heavy use of hooks.

    public private(set) string $logicalPath {
        get => $this->logicalPath ??= $this->parsedFolder->logicalPath;
    }

    private PageSet $children {
        get => $this->children ??= new BasicPageSet($this->pageTree->folderAllPages($this->logicalPath));
    }

Fun fact, I don't think I'm using the long-form version at all yet. 😄 Or the set hook.

I find the above approach completely readable and logical. If we make the outer braces own-line:

    public private(set) string $logicalPath
   {
        get => $this->logicalPath ??= $this->parsedFolder->logicalPath;
    }

    private PageSet $children
    {
        get => $this->children ??= new BasicPageSet($this->pageTree->folderAllPages($this->logicalPath));
    }

That doesn't actually feel any more readable to me. It just feels like more scrolling, especially if I had more than two properties on this class. (I have another that has 10, but they're all inlineable.)

[Crell]

Actually here's a better example from the same project:

class PageRecord
{
    public int $title {
        get => $this->values(__PROPERTY__)[0];
    }

    public int $order {
        get => \max($this->values(__PROPERTY__));
    }

    public bool $hidden {
        get => array_all($this->values(__PROPERTY__), static fn($x): bool => (bool)$x);
    }

    public bool $routable {
        get => array_any($this->values(__PROPERTY__), static fn($x): bool => (bool)$x);
    }

    public bool $isFolder {
        get => array_any($this->values(__PROPERTY__), static fn($x): bool => (bool)$x);
    }

    public \DateTimeImmutable $publishDate {
        get => \max($this->values(__PROPERTY__));
    }

    public \DateTimeImmutable $lastModifiedDate {
        get => \max($this->values(__PROPERTY__));
    }

    public string $pathName {
        get => substr($this->logicalPath, strrpos($this->logicalPath, '/') + 1);
    }

    public array $tags {
        get => array_values(array_unique(array_merge(...$this->values(__PROPERTY__))));
    }

    public function __construct(
        public string $logicalPath,
        public string $folder,
        public array $files,
    ) {}

    private function values(string $property): array
    {
        return array_column($this->files, $property);
    }
}

Adding another line break in there gives me no value, it just makes me scroll more.

[TimWolla]

The new order is much better, thank you.

[TimWolla]

I'm not sure if I agree with the MUST here, because that effectively requires the user to use the short-form and thus is inconsistent with hooks themselves.

I find it reasonable to write the following in an interface (using your brace placement):

public string $both {
    get;
    set;
}

And then shortening that to a single line MAY be done (consistently with hooks).

[Crell]

There's actually a subtle difference here. For interfaces, there can never be a body to a hook, so I see no point in the extra lines. (If Levi ever re-proposes interface default methods that may change, but we'll cross that bridge when we get to it.)

For abstract classes, though, one could specify one hook as abstract and one as predefined. In that case, multi-lining is probably better.

We should probably address those separately.

[TimWolla]

so I see no point in the extra lines

The point is allowing for an uniform formatting of similar things. In this case: A property declaration with a list of “operations”.

I also find it much easier to scan the code vertically rather than horizontally, because the of the established pattern of “a line-break indicates that something new begins, with indentation indicating the relationship to the previous line”.