-
Notifications
You must be signed in to change notification settings - Fork 26
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
Specify hooks formatting #108
base: master
Are you sure you want to change the base?
Conversation
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.
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.
Thoughts now? |
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.) |
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. |
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 new order is much better, thank you.
Co-authored-by: Tim Düsterhus <[email protected]>
Co-authored-by: Tim Düsterhus <[email protected]>
Co-authored-by: Korvin Szanto <[email protected]>
It's great to see progress being made on this--not least because it seems to line up reasonably well with my approach to property hooks in pretty-php (hardly surprising given how PSR-12- and PER-influenced this little I just formatted the most recent code listings from this PR with Otherwise, my only concern so far relates to the spacing of the long form of hooks, e.g. after class Example
{
public string $newName = 'Me' {
set(string $value) {
//
}
}
} My approach has been to format the short form like an arrow function, and the long form like an anonymous function (as opposed to a function named class Example
{
public string $newName = 'Me' {
set (string $value) {
//
}
}
} I do apologise if this has already been covered--I may not have expanded every thread--but I didn't see it mentioned. (And with that, I should probably ask if there is somewhere I should go to introduce myself properly? Long-time stalker, first time talker, looking to engage and connect more as part of getting out of an extended period of isolation and health difficulties, of which |
I adjusted the language for abstract properties, to handle those with bodies separately from those with bodies. I think that covers it. |
@lkrms Welcome! I don't think anyone else has mentioned that so far. I'm not sure I'm a fan, because it's one more thing to change when moving from a short to long form body. Though in practice it should be rare, since 90% of the time you won't need to specify the argument anyway and can rely on the default. (The only reason to specify it is to widen the type, frankly.) Anyone else have an opinion here? There's no real "proper" introduction. Just github issues, and the Discord channel. You're welcome to stop by there and chat if you are so inclined. (Link is in the footer of the FIG website.) I try to keep any formal decision making to issues, but obviously that's always imperfect. |
I generally prefer having the curly brace on the declaration line (to me it feels like it's always just about scrolling more 😉) but since the convention is otherwise to have on its own line this feels a bit weird |
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