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

RFC: match statement #560

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

Conversation

bolinfest
Copy link
Contributor

@bolinfest bolinfest commented Apr 2, 2022

As an initial attempt, I first made a small update to initially set a global in room02_bridge.esc:

:setup
# TODO: only set this if not yet set?
set_global r2_look_dialog_advance 0

which then makes the :look command in button.esc a bit cleaner when using the new match statement:

:look
>> r2_look_dialog_advance
	0 =>
		say player "That button must activate the bridge."
		inc_global r2_look_dialog_advance 1
	1 =>
		say player "I already said that this button must activate the bridge."
		inc_global r2_look_dialog_advance 1
	2 =>
		set_angle player 180
		say player "Listen closely."
		say player "This"
		say player "button"
		say player "must"
		say player "activate"
		say player "the bridge."
		inc_global r2_look_dialog_advance 1
	3 =>
		say player "I give up."
		say player "<sob>"

For the moment, to avoid introducing a new keyword (which would take away a possible command name), >> is used to indicate the start of a match statement. As you can see, the condition arms use a => syntax like Rust (and a number of other languages that support pattern matching).

Arguably, using a match statement avoids repeating the comparison valuable, eliminates the need for stop in many cases, and makes it easier to see that all cases are covered.

@bolinfest bolinfest force-pushed the rfc-match-statement branch 2 times, most recently from fdb855d to 3b61e59 Compare April 2, 2022 20:04
@bolinfest bolinfest force-pushed the rfc-match-statement branch from 3b61e59 to 490fdef Compare April 2, 2022 20:05
@bolinfest
Copy link
Contributor Author

Admittedly, this is not "real" pattern matching. It's more of a glorified switch statement, but it's a start. Honestly, I didn't realize how flexible match is in GDScript today: https://docs.godotengine.org/en/stable/tutorials/scripting/gdscript/gdscript_basics.html?highlight=match#match.

match_lines.size()
)
var pairs = match_expr.parse(match_lines)
escoria.logger.trace("%d arms found for match" % pairs.size())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename "arm" to something like "condition_option" please? It's not very intuitive.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or "branch".

Copy link
Collaborator

@balloonpopper balloonpopper left a comment

Choose a reason for hiding this comment

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

Assuming this gets the green light, please update the documentation as part of the PR.

@balloonpopper
Copy link
Collaborator

Also, is there any way to add conditional logic to this? i.e. issue 220

@StraToN
Copy link
Collaborator

StraToN commented Apr 6, 2022

Very good improvement to the language. I have nothing particular to add about it, apart that obviously documentation is required in the code and some TODOs need to be adresse.

Otherwise, great addition, thanks!

Comment on lines +193 to +195
else:
# TODO: error?
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
else:
# TODO: error?
pass

I'd simply remove this. match_lines.size() > 0 is really just a safety guard. If it really is empty, then we don't care.

# A RegEx identifying a match block
const REGEX = '^([^>]*)>>\\s*(?<expr>.+)$'

const MATCH_ARM_REGEX = '^\\s*(?<cond_expr>.+)\\s+=>\\s*$'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a doc comment to this as well.

return ESCExecution.RC_OK


func parse(lines: Array) -> Array:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a doc comment



func run() -> int:
# Must ensure that at most one condition is satisfied.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part actually should be put into is_valid I think.

match_lines.size()
)
var pairs = match_expr.parse(match_lines)
escoria.logger.trace("%d arms found for match" % pairs.size())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or "branch".

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

Successfully merging this pull request may close these issues.

4 participants