-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Part 2] Modernise code #92
Conversation
$this, | ||
'mapFieldsToIds', | ||
], | ||
$this->mapFieldsToIds(...), |
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.
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.
interesting indeed! Apparently it's a first class callable:
same semantics as Closure::fromCallable()
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.
All good, not much to add.
I guess there is a bit of risk with the added type, in case we weren't following exactly what we're supposed to have. But it's good to have them and we can hope the tests would catch those cases before going to prod 🤞
"name": "Ryan Maber", | ||
"email": "ryan.maber@mention-me.com" |
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.
definitely controversial! 😉
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.
This is the most important part of this PR! 😜
Description
Continuation of: #91
This does:
Testing Notes
Will hook this up to the platform locally before merging and test both Part 1 and 2 together. Once thats done, I'll get merging!