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 switch function #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add switch function #7

wants to merge 1 commit into from

Conversation

sabihoshi
Copy link

This adds a switch function to the JagTag.

{switch:{args}|cases:foo|bar|baz|thing|default:abc}

The default is required, and the cases need to be the same amount, otherwise, the default is returned if the number of cases and values return are not equal.

@Sanduhr32
Copy link

Yeah what about multiple cases using the same branch? like

switch(xxxx) {
   case a:
   case b:
      blah();
      break;
   default:
       dude(pls, why);
       break;
}

@sabihoshi
Copy link
Author

sabihoshi commented Feb 17, 2020

There's a few ways to do it, it could be

{switch:{args}|cases:[a,b]|bar|baz|thing|default:abc}

Or maybe something like

{switchregex:{args}|cases:(a|b)|bar|baz|thing|default:abc}

To be honest the syntax can be anything to fit here, but I wanted to follow how the original syntax for the language looked. Do you have a suggesetion?

@jagrosh
Copy link
Owner

jagrosh commented Feb 18, 2020

I'm not super concerned with fallthrough, one thing that I don't think is clear from the syntax though is whether:

{switch:{args}|cases:a|b|c|d|default:e}

is

case a: b
case c: d

or

case a: c
case b: d

I'm trying to think of syntax improvements, but I'm not quite sure which direction to go. Also, I think a "closest" version of switch would be very useful, where instead of looking for an exact case name and using the default if not found, it used whichever case name was the "closest," (based on some calculation), or default if nothing matched within a certain threshhold. This would probably use nearly identical syntax, albeit different code internally.

@sabihoshi
Copy link
Author

sabihoshi commented Feb 20, 2020

This was brought up because we were running out of space doing if statements and the syntax are like that just to be able to save more space. It does the first option, where the thing to execute is immediately after the case, like how a common switch works.

I was thinking of just using the statements from if, but I think it should be something that's applied as a whole to make it easier, as per case matching of the modifier would be too messy.

{switch:{args}|~|
    test|hello|
    foo|bar|
    baz|plum|
    default
}```

@jagrosh
Copy link
Owner

jagrosh commented Feb 20, 2020

Case-insensitive matching can be done like this (also lower the cases if not provided in plaintext):

{switch:{lower:{args}}|cases:case1|val|case2|val2|default:def}

I definitely want to see this added, I just want to make sure that it's set up in a way that's consistent with what exists, and will be able to be expanded upon later.

Also, while not quite as concise as this new switch statement, you can currently do something like:

{set:case1|val1}{set:case2|val2}{set:case3|val3}{get:{args}}

which might be a bit more-readable than nested if-statements, albeit more difficult to do default values.

@sabihoshi
Copy link
Author

sabihoshi commented Feb 22, 2020

Well anyways, with this it's good enough, I don't think we need to setup the switch in a way we could use the if statements, I think I should implement an {eval} which spits out a true/false. For that, we are able to do

{switch:true|{eval:{args}|>|90}|Excellent!|{eval:{args}|>|50}|Good|default:Bad}```

@sabihoshi
Copy link
Author

I'm reading the AppVeyor log and can't figure out what's failing the build



// performs a conditional and outputs true/false
new Method("eval", (env, in) -> {

Choose a reason for hiding this comment

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

I feel like this isn't solving the issue.

Copy link
Author

@sabihoshi sabihoshi Feb 22, 2020

Choose a reason for hiding this comment

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

Why not? I think case fallthrough can be solved by the eval method, I just need one more function, which would be {logic:&|{eval}|{eval}}

Anything else than this differs too much from the original language, which I wanted to replicate as much as possible.

Choose a reason for hiding this comment

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

eval might be possible to exploit. Thats just why... you know?

Copy link
Author

Choose a reason for hiding this comment

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

It's not an actual !eval from anywhere. It's the same evaluator that {if} uses. Maybe take a look at the code first? :)

Copy link
Owner

Choose a reason for hiding this comment

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

I actually don't think eval should be part of this PR; it doesn't seem related to switch statements, and I'd rather it be called something like boolean or conditional, and be added at the same time as logic

Copy link
Author

Choose a reason for hiding this comment

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

That's a fair point. I'll move it to another PR perhaps.

@sabihoshi
Copy link
Author

sabihoshi commented Feb 28, 2020

If a multiple case scenario was really needed, I feel like arrays could solve the issue much better.

{switch:{args}|cases:
    ["foo","bar"]|result1|
    baz|result2|
    default:bass
}

But this sort of thing (arrays) aren't implemented yet, so I will come back to it once they're done.
Right now, I think one way to do it is just using replaceregex the argument into a common pattern, or do it by doing

{switch:true|cases:
    {bool:{replaceregex:(foo|bar)|with:true|in:{args}}|=|true}}|result1|
    {bool:{args}|=|baz}|result2|
    default:bass
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants