-
Notifications
You must be signed in to change notification settings - Fork 6
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
Getting user information #4
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4 +/- ##
===========================================
- Coverage 3.53% 3.49% -0.05%
Complexity 145 145
===========================================
Files 30 30
Lines 452 458 +6
===========================================
Hits 16 16
- Misses 436 442 +6
Continue to review full report at Codecov.
|
Could you add some test cases for |
@gundamew like that bro? |
… => 'Not found' will set user only userId.
src/LineDriver.php
Outdated
], true); | ||
|
||
$userInfo = json_decode($userInfoData->getContent(), true); | ||
$names = self::split_name($userInfo['displayName']); |
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.
https://developers.line.me/en/reference/messaging-api/#get-profile
I checked the response columns of LINE profile API. It only contains displayName
and no other "name"s.
For reference, the user data returned from Telegram API. The last_name
column is optional, so it is fine if user only set first name.
IMO maybe we don't have to split user names? Just display the value of displayName
column.
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.
Sure. just use displayName value as fist_name.
src/LineDriver.php
Outdated
|
||
return new User($userId, $names['first_name'], $names['last_name'], null, $userInfo); | ||
} catch (\Exception $e) { | ||
return new User($userId); |
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.
Uh, where the exception being thrown?
And IMO, the method should handle the exceptions, instead of just returning another User
instance.
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.
So I read https://github.com/botman/driver-slack/blob/master/src/SlackDriver.php and follow how they code the function. What do you think?
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.
https://github.com/botman/driver-slack/blob/master/src/SlackDriver.php#L386-L403
I had read the code of BotMan Facebook driver and Telegram driver but not Slack driver. It is interesting.
I got your point, but I still prefer to do things simple. If error happens, let it happen.
No description provided.