-
Notifications
You must be signed in to change notification settings - Fork 187
Foreach completion #551
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
Foreach completion #551
Conversation
Codecov Report
@@ Coverage Diff @@
## master #551 +/- ##
===========================================
+ Coverage 80.86% 81.5% +0.64%
- Complexity 877 898 +21
===========================================
Files 61 61
Lines 2028 2055 +27
===========================================
+ Hits 1640 1675 +35
+ Misses 388 380 -8
|
| if ($collectionType instanceof Types\Array_) { | ||
| return $collectionType->getKeyType(); | ||
| } | ||
| return new Types\Mixed_(); |
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.
code style; no brackets at the end.
|
@phil-nelson can you please take a look at #561 could be related to your PR on foreach completion. <?php
foreach($Calendar as $day)
echo $day->Date->format("D");While, this one is fine: <?php
foreach($Calendar as $day)
echo $day->format("D"); |
This adds some basic support for completing
foreachkeys/values. For a complete list of what works have a look at the test, but I believe most "normal" cases work.Limitations:
list()for values is unsupported. This isn't straightforward, and as far as I can tell we don't support a straight uplist()(outside a foreach) for completion\TraversableobjectsI think implementing these limitations is probably more significant work than what's here. I think the cases that work are probably the most used/popular, so even given these limitations I think this would be worth having. We can work on the missing bits in the future.
Closes #200