Skip to content

♻️ doc types first pass #994

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

Merged
merged 2 commits into from
Jan 23, 2025
Merged

♻️ doc types first pass #994

merged 2 commits into from
Jan 23, 2025

Conversation

willpower232
Copy link
Contributor

A first pass of doc types, referencing the v14 branch as well as phpstan level 8, removing duplicated types and adding missing types.

I'm hoping this is basic enough to not cause any issues.

@willpower232 willpower232 requested a review from erikn69 January 18, 2025 16:45
@willpower232 willpower232 self-assigned this Jan 18, 2025
@willpower232
Copy link
Contributor Author

spoiler alert php 7 doesn't support this so would have to wait for v14 I guess

image

src/Audit.php Outdated
*/
public function getTags(): array
public function getTags()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not ?:

return preg_split('/,/', $this->tags, -1, PREG_SPLIT_NO_EMPTY) ?: [];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I would rather allow the user to detect failure since there could be a big problem with whatever the user is trying to do and covering it up with an empty array would effectively silence any issue unless the user is paying close attention to their output.

could always check for the false and throw an exception I guess which would be a bit clearer to the user than the type error which would probably happen currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But within this context that should not happen(no problems reported), because those tags come from the database, where the audit was saved, and correctly formatted, am I wrong?

This is just a change to make phpstan happy in my opinion, I think we should avoid complicating things for developers.
It seems to me that it is changing the signature unnecessarily

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah absolutely, phpstan is quite brutal. I've just got a lot of years of not trusting anyone haha.

I'll simplify it 🙏

@erikn69 erikn69 force-pushed the doc-types-first-pass branch from 0a99ae0 to 011fb8c Compare January 23, 2025 14:46
@erikn69 erikn69 merged commit 4a212b1 into master Jan 23, 2025
17 checks passed
@erikn69 erikn69 deleted the doc-types-first-pass branch January 23, 2025 14:56
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.

3 participants