Skip to content
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

🚀 Add player(nameOrUUID: string, getExactPlayer: boolean) function #5845

Merged
merged 9 commits into from
Aug 21, 2023

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Jul 17, 2023

Description

Adds a player function that return player or offlineplayer from name or uuid.

+ player(nameOrUUID, getExactPlayer) :: player # returns none if player is offline
+ offlineplayer(nameOrUUID) :: offlineplayer # returns none only if player does not exist in Minecraft usernames database (not taken)

Target Minecraft Versions: Any
Requirements: Any
Related Issues:

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Jul 17, 2023
@Fusezion
Copy link
Contributor

Fusezion commented Jul 29, 2023

Was looking over this in the issue the suggested syntax was to do player(nameOrUUID, onlineOnly) is it still planned to make it support online only or was it just weird to see it used? and if not should we do offlinePlayer and player in 2 functions instead?

I understand if not as it can be a bit redundant since is online isn't a hard condition to use after

@AyhamAl-Ali
Copy link
Member Author

Was looking over this in the issue the suggested syntax was to do player(nameOrUUID, onlineOnly) is it still planned to make it support online only or was it just weird to see it used? and if not should we do offlinePlayer and player in 2 functions instead?

I understand if not as it can be a bit redundant since is online isn't a hard condition to use after

The main idea behind adding isOnline param was to avoid erroring when skript asks for player and finds offlineplayer however, this wasn't correct as Skript has offlineplayer -> player converter so it works well

This was the test I made first. IMO currently it's good how it is but if you have some use cases feel free to share them here
image

@Fusezion
Copy link
Contributor

This was the test I made first. IMO currently it's good how it is but if you have some use cases feel free to share them here

Nope just wanted to make sure as I didn't see it mentioned, reasoning makes perfect sense to me.

@Pikachu920
Copy link
Member

what do you think about using two functions (player and offlineplayer). that way, you don't have to remember what the boolean parameter means (plus it's more clear without looking at the docs).

@Fusezion
Copy link
Contributor

Fusezion commented Aug 1, 2023

support online only or was it just weird to see it used? and if not should we do offlinePlayer and player in 2 functions instead?

what do you think about using two functions (player and offlineplayer). that way, you don't have to remember what the boolean parameter means (plus it's more clear without looking at the docs).

was something I mentioned here as well, but it falls under the same thing as it's as simple as doing {_player} is online it just doesn't flow as well. Player as a standalone tells me hey this function returns a player object

@sovdeeth
Copy link
Member

sovdeeth commented Aug 1, 2023

personally i'd like player(nameOrUUID, allowOffline), I think defaulting to online only is safer.
Having two functions would also follow the rule of only doing offline lookups if you tell it to, but i think would be a bit more cumbersome. You make a good point about remembering the argument, though, pikachu.

@AyhamAl-Ali
Copy link
Member Author

what do you think about using two functions (player and offlineplayer). that way, you don't have to remember what the boolean parameter means (plus it's more clear without looking at the docs).

This falls under the same subject Fuze mentioned, read this reply and let me know what you think

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional addition could be a boolean flag to use getExactPlayer instead of just getPlayer. I know there have been a few complaints that parsing "a" as player will match "Andre", if Andre 's online and a isn't.

@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Aug 2, 2023

Optional addition could be a boolean flag to use getExactPlayer instead of just getPlayer. I know there have been a few complaints that parsing "a" as player will match "Andre", if Andre 's online and a isn't.

Not sure about this addition, is there any useful use case for it? I am afraid users will just never use it OR misuse it and complain about their code not working as expected

@sovdeeth
Copy link
Member

sovdeeth commented Aug 2, 2023

Optional addition could be a boolean flag to use getExactPlayer instead of just getPlayer. I know there have been a few complaints that parsing "a" as player will match "Andre", if Andre 's online and a isn't.

Not sure about this addition, is there any useful use case for it? I am afraid users will just never use it OR misuse it and complain about their code not working as expected

Say you have a stored list of names (or a text input from chat, or a sign, or something) and you need the player objects of the online players. You can:
a) parse all as player, which is cheaper but risks mis-parsing some names as other players
b) parse all as offlineplayer, and check who's online, more expensive but more reliable.

Additionally, this issue can and does pop up when using arguments in commands, but that's not related to this PR.

It'd be a niche and pretty minor addition, but something i'd appreciate and have seen a few people grapple with in SkUnity help channels.

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid!

@AyhamAl-Ali AyhamAl-Ali changed the title 🚀 Add player(nameOrUUID: string) function 🚀 Add player(nameOrUUID: string, getExactPlayer) function Aug 2, 2023
@AyhamAl-Ali AyhamAl-Ali changed the title 🚀 Add player(nameOrUUID: string, getExactPlayer) function 🚀 Add player(nameOrUUID: string, getExactPlayer: boolean) function Aug 3, 2023
@AyhamAl-Ali AyhamAl-Ali merged commit 7b0bfce into SkriptLang:master Aug 21, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants