-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
non-camel-case variable code conventions (with extra tests) #57
non-camel-case variable code conventions (with extra tests) #57
Conversation
Failing tests are only on |
@@ -45,12 +45,10 @@ public static function getObjectFieldValue($object, $field) | |||
if (is_array($object)) { | |||
return $object[$field]; | |||
} | |||
|
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.
please don't remove these empty lines
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.
Ah my bad, I was debugging a bit in a particular test case, I will revert this on Monday.
the HHVM-nightly builds are now removed |
What needs to be done here? |
…ExpressionVisitor
I actually forgot to provide the last patch, which is added now. @Ocramius is there anything this PR is still missing in order to get the fix through? |
@mikeSimonson You have our thanks! |
@mikeSimonson Do you have some info when there will be a release that includes this pull? |
I need to fix a few stuff that I pulled here before to release. I am not making any promise but I hope soon. |
okay cool! looking forward to it |
This is not enough. Suppose my field is called |
@maresja1 What you are suggesting would be a great addition. However, this would require an extension point for the |
Because people keep running into issues with collections and
$underscore_properties
, I have added extra tests on the changes of #29 to to prove that it's backwards compatible. It seems like that one won't be merged because the test cases are missing.This PR includes the exact same changes as #29 but with 3 extra tests.