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

style: Enable phpdoc_list_type fixer #8530

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

paulbalandan
Copy link
Member

Description
Enables phpdoc_list_type, changing array phpdocs with implicit keys to list, e.g. array<string> => list<string>. Then let phpstan catch incorrect usages.

If approved, don't merge yet so this can be applied in the coding standard repo and minimize updating the overrides here.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@github-actions github-actions bot added the stale Pull requests with conflicts label Feb 10, 2024
Copy link

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

@kenjis
Copy link
Member

kenjis commented Feb 10, 2024

@paulbalandan Sorry, I've changed to use @property-read. See #8519.
Can you update coding-standard with that?

@kenjis
Copy link
Member

kenjis commented Feb 10, 2024

phpdoc_list_type is risky.
https://cs.symfony.com/doc/rules/phpdoc/phpdoc_list_type.html

I'm not sure we should enable this rule by default.

@paulbalandan
Copy link
Member Author

Yes, it is risky by the reasoning behind it.

If we make a phpdoc like Foo[], I think we almost always mean the same way when we push to an array, like $a[] = Foo;, so Foo[] here would mean a list of Foo, i.e., array<int, Foo>. If we mean a string key, we should have indicated it like array<string, Foo>, right?

@kenjis kenjis removed the stale Pull requests with conflicts label Feb 14, 2024
app/Config/View.php Outdated Show resolved Hide resolved
system/Config/View.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Feb 14, 2024

If we make a phpdoc like Foo[], I think we almost always mean the same way when we push to an array, like $a[] = Foo;, so Foo[] here would mean a list of Foo, i.e., array<int, Foo>. If we mean a string key, we should have indicated it like array<string, Foo>, right?

I checked all the changes in this PR, and you are right.
I agree that most PHP devs think like that.

I agree to enable phpdoc_list_type.

@paulbalandan paulbalandan force-pushed the phpdoc-list-type branch 4 times, most recently from fa9e27d to e37b8a3 Compare February 16, 2024 15:35
@paulbalandan paulbalandan marked this pull request as draft February 24, 2024 13:57
@paulbalandan
Copy link
Member Author

To be merged once latest coding-standard is out.

@paulbalandan paulbalandan marked this pull request as ready for review February 25, 2024 16:18
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

The Psalm errors are not directly related to this PR.

@kenjis kenjis merged commit a718427 into codeigniter4:develop Feb 26, 2024
61 of 62 checks passed
@kenjis kenjis mentioned this pull request Feb 26, 2024
3 tasks
@paulbalandan paulbalandan deleted the phpdoc-list-type branch March 10, 2024 14:52
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