I saw on Laravel-news a post about Spatie guidelines. Because other peoples guidelines are always a chance to learn something new, I thought it would be fun to read through them and provide comments.
I'm going to focus on the PHP/Laravel guidelines.
The PHP agent skill is split up in two files where most of the information in SKILL.md is repeated in spatie-laravel-php-guidelines.md. This seems a waste of tokens.
Use camelCase for non-public-facing strings
I assume the model(s) don't always follow that rule of PSR-12. It is not explicitly mentioned, so it can only be inferred by reading the examples.
Use short nullable notation: ?string not string|null
This is explicitly mentioned in PSR-12.
I understand the reasoning behind the rule because of the other PHP functionality using the question mark to signal null like $client?->address and $client ?? new Client().
My own take is to use the long notation to be consistent with the cases where null is a part of three or more options, string|int|null. It is one less place where your mind needs to make the switch that a question mark means null.
Use typed properties, not docblocks
I'm all for documenting using code. It is one of the reasons I'm against using array shapes in docblocks. When the shape is that important use a type.
Use constructor property promotion when all properties can be promoted
With the existence of a variable arguments list this is a bit of a tricky guideline.
A variable arguments list is a more solid way to pass input than an array.
So when a model follows that rule, all none variable properties aren't going to get promoted?
Document iterables with generics:
/** @return Collection<int, User> */
A better way would be to create a specific collection type for the Eloquent model output. It used to be more (too much?) effort but with code generation it is not going to matter.
Always import classnames in docblocks; never use fully qualified names:
This is good advice!
Happy path last: handle error conditions first, success case last
While it is a good rule, it is not always possible.
if (! $user) {
return null;
}
if (! $user->isActive()) {
return null;
}
In the same code block with ternary examples this looks very drawn out.
if($user?->isActive() === null) {
return null;
}
I understand it is an example of multiple if's over multiple branches. But reducing the if's in a function is also a good goal.
// Ternary instead of else
$condition
? $this->doSomething()
: $this->doSomethingElse();
For me this looks like using match when it is better to use switch.
While the ternary operator is not explicitly bound to a return. The PHP documentation suggests that is the most likely use case.
Else isn't that bad that it needs to be avoided by any means.
Use tuple notation: [Controller::class, 'method']
This assumes the controller is very likely to have constructor dependency injection, while it is better to use method dependency injection.
The method dependency injection makes it also possible to use new Controller()->method(...) which is more robust than the method as a string.
In the few cases constructor dependency injection is the best solution, the tuple notation can be used.
Stick to CRUD methods (index, create, store, show, edit, update, destroy)
I understand the rule from a consistency perspective. On the other hand it is very restrictive.
When the controllers are only CRUD vessels, is Laravel the right solution?
Use config() helper, avoid env() outside config files
Solid rule!
Put output before processing an item (easier debugging):
$items->each(function (Item $item) {
$this->info("Processing item id `{$item->id}`...");
$this->processItem($item);
});
$this->comment("Processed {$items->count()} items.");
It assumes there is a build-in way to add debug information. As far as I know that is not default.
Class constants also use PascalCase:
This is not compatible with PSR-2.
Conclusion
An agent skill is hard to get right because different models can interpret information in another way.
The skills will work with the model(s) Spatie is using, and that is why some of the guidelines may look a bit out of place.
Like with any other guidelines, they are opinionated. And because we all have our own opinions there are parts where we don't agree.
But the nice thing about agent skills is that they are easy to change to your own liking.
Top comments (0)