Skip to content
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

Add @psalm-* annotations for better static analysis (compatible with PHPStan too) #97

Merged
merged 2 commits into from
Mar 23, 2024

Conversation

gnutix
Copy link
Contributor

@gnutix gnutix commented Mar 22, 2024

Below are the two types improvements I've noticed were missing when we've integrated the library in our application. I've not read every code signature to make sure everything was as precise as it could be though, so maybe other type improvements will come in the future.

TODO

  • @return non-empty-string on toString methods (and alike)
  • @return -1|0|1 on compareTo methods

@gnutix gnutix force-pushed the add-more-psalm-annotations branch from 2c7dc72 to bf98faa Compare March 22, 2024 10:02
@gnutix gnutix changed the title Add @psalm-* annotations for better static analysis Add @psalm-* annotations for better static analysis (compatible with PHPStan too) Mar 22, 2024
@gnutix gnutix marked this pull request as ready for review March 22, 2024 11:01
@BenMorel
Copy link
Member

Hi, good point, I actually added @psalm-return -1|0|1 to compareTo() in brick/math, but forgot to do the same here!

I'm curious about non-empty-string though, I've never come across a use case where this would be useful. Do you have an actual example code where knowing that the output of say toISOString() would help Psalm? I fail to see a real life use case where I'd check this against an empty string!

@gnutix
Copy link
Contributor Author

gnutix commented Mar 23, 2024

An example that comes to mind would be indexing an array of Brick objects (let's say a LocalDate) by their ISO strings. If I have a function indexBy(array<T>, callable(T): non-empty-string): array<non-empty-string, T>, I want to make sure in the signature that it does not return an empty string, as indexing an array using one would produce unexpected results (the value would be overridden for each entry, and only the last one would remain).

Or if you have a method that expects some kind of "identifier", which could have invariants making sure it's not empty.

Sorry if it's not the most amazing use cases I'm thinking of right now, but that's something. ;)

@BenMorel
Copy link
Member

BenMorel commented Mar 23, 2024

Indeed that could make sense here. And it doesn't hurt ;-) Thanks for the example!

@BenMorel
Copy link
Member

BenMorel commented Mar 23, 2024

Please rebase to fix CS and I'll merge this! 👍

Edit: looks like @psalm-return non-empty-string is missing on LocalDateRange?

BenMorel
BenMorel previously approved these changes Mar 23, 2024
@BenMorel BenMorel dismissed their stale review March 23, 2024 00:31

Missing @psalm-return non-empty-string on LocalDateRange

@gnutix gnutix force-pushed the add-more-psalm-annotations branch from bf98faa to fe52b56 Compare March 23, 2024 08:42
@gnutix gnutix requested a review from BenMorel March 23, 2024 09:17
@BenMorel BenMorel merged commit c780a8f into brick:master Mar 23, 2024
7 checks passed
@BenMorel
Copy link
Member

Thank you, @gnutix!

@BenMorel BenMorel added this to the 0.7 milestone Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants