Skip to content

Add switch function - #7

Open
sabihoshi wants to merge 1 commit into
jagrosh:masterfrom
sabihoshi:master
Open

Add switch function#7
sabihoshi wants to merge 1 commit into
jagrosh:masterfrom
sabihoshi:master

Conversation

@sabihoshi

Copy link
Copy Markdown

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
Copy Markdown

Yeah what about multiple cases using the same branch? like

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

@sabihoshi

sabihoshi commented Feb 17, 2020

Copy link
Copy Markdown
Author

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

jagrosh commented Feb 18, 2020

Copy link
Copy Markdown
Owner

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

sabihoshi commented Feb 20, 2020

Copy link
Copy Markdown
Author

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

jagrosh commented Feb 20, 2020

Copy link
Copy Markdown
Owner

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

sabihoshi commented Feb 22, 2020

Copy link
Copy Markdown
Author

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
Copy Markdown
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) -> {

Copy link
Copy Markdown

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.

@sabihoshi sabihoshi Feb 22, 2020

Copy link
Copy Markdown
Author

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.

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

sabihoshi commented Feb 28, 2020

Copy link
Copy Markdown
Author

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