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 EffDoIf #1536

Merged
merged 12 commits into from
Oct 12, 2018
Merged

Add EffDoIf #1536

merged 12 commits into from
Oct 12, 2018

Conversation

OfficialDonut
Copy link
Contributor

Target Minecraft versions: any
Requirements: none
Related issues: none

Description:
This executes an effect if a condition is true. The syntax is [(do|execute)] <.+> if <.+> where the first regex is an effect and the second is the condition.
Ex:

on join:
	give diamond to player if player has permission "rank.vip"

Standalone this effect does not conflict with ternaries but if a ternary is used within the effect parenthesis must be used:

#invalid
give diamond if {diamond::%player%} is true else emerald to player if player has permission "rank.vip"

#valid
give (diamond if {diamond::%player%} is true else emerald) to player if player has permission "rank.vip"

@Blueyescat
Copy link
Contributor

Blueyescat commented Sep 11, 2018

Nice idea! I was thinking about this lol.

I think [(do|execute)] should be removed because it would look ugly and doesn't make sense because this is not something like evaluate %string%.

execute stop
do ban the player
execute execute player command ""
do make player ride target

Also what happens if you nest do ifs?

stop if {_x} set if {_y} is not set
(stop if {_x} set) if {_y} is not set

Both shouldn't work since it uses regex and already it shoudn't work i think.

@OfficialDonut
Copy link
Contributor Author

Yeah I agree the do/execute part doesn't sound very good and yeah nested ifs don't work

@Pikachu920
Copy link
Member

I'm not sure if I like this idea, it's not like the alternative is much harder to write. The parser is already not so good at handling long lines.

A couple of things ignoring my concerns:

  1. The code won't compile using ECJ
  2. These shouldn't be nestable

@OfficialDonut
Copy link
Contributor Author

they're not nestable

@Pikachu920
Copy link
Member

I don't see where in the code you enforce that

@OfficialDonut
Copy link
Contributor Author

it doesn't work
do (do set {test} to 1 if 1 is 1) if 1 is 1
is there a case where it does?

@Pikachu920
Copy link
Member

Pikachu920 commented Sep 12, 2018

you shouldn't just rely on the parser being dumb, though... best to check in init

@Blueyescat
Copy link
Contributor

Blueyescat commented Sep 12, 2018

@Pikachu920 The parser will read the code from the right and it uses regex

So when you use set {test} to 0 if 1 is 1 if 2 is 2
set {test} to 0 will be the DO
1 is 1 if 2 is 2 will be the IF (which is a wrong condition and will error)

And in this case you can't understand if the first regex is also a Do If effect.


BTW (set {test} to 1 if 1 is 1) if 1 is 1 is not right, you can't use parenthesis around a regex.

@OfficialDonut
Copy link
Contributor Author

Pikachu is right it doesn't hurt to guarantee it

@Blueyescat
Copy link
Contributor

That change won't affect anything.

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Sep 17, 2018

Define what you're changing in the title of the git commit, rather than just Update EffDoIf.java

After the if there should be support for all genders and nouns that Skript already supports in syntax. Test to make sure it does this already from the expressions. If not that will need to be added.

Examples:

teleport player to the target entity if the target entity is a sheep
set target block to light green stained glass if a lamp is above the player

Also have you tested if time states work?

on fade:
    cancel event if the block was ice

In my opinion I like IIF/ternary better but both are grammatically correct for an English based scripting language.

on fade:
    if the block was ice cancel the event

@Blueyescat
Copy link
Contributor

@TheLimeGlass Your examples will work the same since he didn't make his own parser.

@Nicofisi
Copy link
Member

Also have you tested if time states work?

on fade:
cancel event if the block was ice

@TheLimeGlass this wouldn't even work when used normally already, #671

@TheLimeGlass
Copy link
Collaborator

Then fix it

@Nicofisi Nicofisi added the feature Pull request adding a new feature. label Sep 30, 2018
@bensku bensku requested a review from Snow-Pyon October 8, 2018 15:44
@bensku bensku merged commit 686454c into SkriptLang:master Oct 12, 2018
@Misio12320
Copy link

When this will be added?

@Romitou
Copy link
Member

Romitou commented Apr 12, 2020

When this will be added?

It's already added.

@ShaneBeee
Copy link
Contributor

When this will be added?

As you can see by the purple "Merged" ... that means it's been added. It was actually added back in 2.3
http://skriptlang.github.io/Skript/effects.html#EffDoIf

@Misio12320
Copy link

Yes. Okay so merged = closed + added now I understand. And what the red is Travis fixes, forgot import, and fixed eff not working?

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.

9 participants