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

Fix message and broadcast with or list, redesign or ExpressionList getting #4545

Merged

Conversation

TPGamesNL
Copy link
Member

@TPGamesNL TPGamesNL commented Jan 29, 2022

Description

Fixed EffMessage and EffBroadcast to handle 'or' expression lists correctly.
Also changed how a few of ExpressionList's methods work (getSingle, getArray and iterator), how the behaviour changed is explained in example below. The rest of the diff is formatting.

command /test:
	trigger:
		broadcast "a", "b" or "c"
		send "A", "B" or "C"
		
		set {_x} to "X"
		set {_y} to {_x} or {_none}
		broadcast "y: %{_y}

Before PR:
image
After PR:
image

ExpressionList would always try to pick a non-null value instead of picking a random value from the list.
I personally think the new behaviour from this PR makes more sense, but I'd like feedback on it (which is why I added 'up for debate' label)


Target Minecraft Versions: any
Requirements: none
Related Issues: #4543

@TPGamesNL TPGamesNL added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question labels Jan 29, 2022
@TheLimeGlass
Copy link
Contributor

TheLimeGlass commented Jan 31, 2022

This doesn't mess up;

on inventory click:
    if index of clicked slot is 1 or 2:
        cancel event

by being randomly selecting one or the other? Making sure we're not introducing a new bug.

Also I think we should add tests for this, to ensure this doesn't break if it's implemented.

@TPGamesNL
Copy link
Member Author

It doesn't break usage of ExpressionList in conditions, because conditions should use Expression#check, which is unaffected from this change.
The only effective change in ExpressionList is that {_x} or {_none} will no longer pick {_x} 100% all the time, as it didn't make sense to me, so I don't think this will break anything beside exactly the behaviour above.

@TheLimeGlass
Copy link
Contributor

Ok, we should definitely add tests to maintain this standard. I can make some when I can if you don't before me.

@TPGamesNL TPGamesNL merged commit 949fd15 into SkriptLang:master Apr 1, 2022
@TPGamesNL TPGamesNL deleted the fix/message-broadcast-or-list branch April 1, 2022 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants