Add the 'target' type of argument for player selections #7
Conversation
|
Getting an offline player isnt the intended behaviour for this argument |
|
Also should it not return the player object so we dont have to make another call to getplayer? |
|
It obviously doesn't return a player object. Read the code. The provided code doesn't get an offline player either. That function creates a fake offline player any time it's called, so I am using it to ensure there are no "function getName() on null" errors. The client will only ever be able to see names of online players but I still want to provide usage for players that dont exist so that they can use the argument as a normal string the way vanilla handles it. |
|
Suggestion: Get the server instance on the constructor and store it on a
property so that we don't need to get it every time? 🤔
…On Mon, Jun 3, 2019, 10:23 AM jasonwynn10 ***@***.***> wrote:
It obviously doesn't return a player object. Read the code.
The provided code doesn't get an offline player either. That function
creates a fake offline player any time it's called, so I am using it to
ensure there are no "function getName() on null" errors. The client will
only ever be able to see names of online players but I still want to
provide usage for players that dont exist so that they can use the argument
as a normal string the way vanilla handles it.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#7?email_source=notifications&email_token=AEHQQFH2GAQJWO4GAM6QKA3PYR6DPA5CNFSM4HSDFH7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWYERAA#issuecomment-498092160>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEHQQFEDSGL5476OZNO3JSLPYR6DPANCNFSM4HSDFH7A>
.
|
|
Isn't that just a micro optimization? |
|
For me, it looks "cleaner" because it's not a long line of code with
duplicated segments that reference back to the same object which could've
been shorter with a variable.
Just a tiny thing that triggers my OCD a bit 😂
…On Mon, Jun 3, 2019, 12:29 PM jasonwynn10 ***@***.***> wrote:
Isn't that just a micro optimization?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#7?email_source=notifications&email_token=AEHQQFHSPMQAYREIPWDLT7DPYSM4NA5CNFSM4HSDFH7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWYIUMY#issuecomment-498108979>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEHQQFFOWD4SESE6GSAFMGLPYSM4NANCNFSM4HSDFH7A>
.
|
|
@jasonwynn10 Look at Cortex's own implementation of something similar to this: https://github.com/CortexPE/Hierarchy/blob/master/src/CortexPE/Hierarchy/command/args/MemberArgument.php This is why i suggested returning the player object. Nowadays we should be checking if the player is null in vanilla pmmp and we can continue doing that with commando. Also, i get that that you would like to support online players but could this not be implemented in a seperate argument, or left to the developer to implement as a custom argument? |
It matches vanilla's behaviour. |
|
@jasonwynn10 ?? |
| public function parse(string $argument, CommandSender $sender) { | ||
| // TODO: handle @a @e @p @r @s @c @v | ||
| $player = $this->server->getPlayer($argument) ?? $this->server->getOfflinePlayer($argument); | ||
| return $player->getName(); |
There was a problem hiding this comment.
Oh, also why does it return the name instead of Player or OfflinePlayer? Wouldn't that be more versatile?
And we don't need to call Server->getPlayer() because Server->getOfflinePlayer() already returns a Player if it's available:
https://github.com/pmmp/PocketMine-MP/blob/stable/src/pocketmine/Server.php#L778-L792
|
If it only needed the name and nothing else, why not just validate the input if it's a valid name? And imho, I really don't see most people using this type of argument assuming that it would return just a name... |
|
I make it find an actual online player for name auto-complete. If there is no one online with that name, then it will not autocomplete. |
|
Oh so autocomplete.... Isn't the name misleading? 🤔 |
|
how is it misleading? |
|
We're usually returning the actual object and usually not just the names... From the name |
|
oh, well I can have it return a player or null without the auto-complete I guess |
|
Or it could be named "PlayerNameArgument" 🤔 |
|
🤔 |
|
Closed in favor of #15 |
No description provided.