-
Notifications
You must be signed in to change notification settings - Fork 672
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
List refactoring v5 #8820
List refactoring v5 #8820
Conversation
Whoops sorry muglug wanted to tag just orklah :) |
I didn't start reviewing this yet. I'm a little worried about possible BC breaks. Wouldn't it be better to make a new sweep of changes and ship it as Psalm 6? We're not obligated to any kind of waiting period between major and it would probably be cleaner, no? |
@danog I'd strongly encourage you to close this PR and reopen one with a single commit (or, if it makes sense, a small number of curated commits). When I review a PR, I generally look at commit messages to figure out what's going on. This PR has 134 commits, which is incredibly noisy (I had to unsubscribe from this PR because it was spamming my inbox). |
0a74397
to
499c6b7
Compare
Sorry about the noise, I just prefer to use GHA to run unit tests, will submit PRs with fewer and more relevant squashed commits in the future. Squashed everything, the main changes are documented in UPGRADING.md, the rest is just a lot of bugfixing for bugs detected using the existing unit tests. |
About BC breaks, I actually wanted to ask about the timeline for the Psalm v6 release/Psalm v5/v6 feature freeze, I really think this should be planned out a bit (like PHP itself does) to avoid an excessively postponed release like for v5. Obviously personally I would love to merge in all BC breaks right away and release v6 (Phpstan has it easier at least visually with 0.x semver), but if that can't be done I can just revert the remaining getGenericArray BC break. |
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.
Just reviewing the test changes (always the easiest part of a large PR to review) saw some things to improve/break out
Do you think it would be OK to merge this? :) |
Mind looking at the |
I already checked them and prepared psalm/endtoend-test-phpunit#7 and psalm/endtoend-test-psl#3 to fix :) |
Thanks! I've just merged those — two remaining errors in the PHPUnit run (btw, don't feel shy about merging PRs yourself in the endtoendtest- repos) |
Hehe I'd love to, but I don't have permission to merge PRs in the e2e or plugin repos in the psalm org. |
I've added you! |
12b9c13
to
3ed9fed
Compare
f2c33a8
to
6eb37b9
Compare
Thanks! |
Fixes a huge number of list-related and non-list-related bugs.