🖊️ Allow -= and > operators to be used independently when merging gra…

…mmars (#5186)

Fixes #5176

Merging grammar rules was only possible if the += operator was present, which means that the -= and > operators could not be used independently of +=. With this change, all operators could be used independently of each other.

Because the > operator now does not need the context of the += operator, I had to change it to a symbol which does not appear in the lark language (e.g. ->). Please consider whether there is a better candidate than `>>`, for example `>=`?

**How to test**

- The introduced merging changes are covered with unit tests
- All existing tests should pass

grammars/level15-Additions.lark

@@ -1,4 +1,4 @@
-command:+= while_loop | ifpressed-= error_ifpressed_missing_else > ifelse | ifs
+command: += while_loop | ifpressed -= error_ifpressed_missing_else >> error_ifelse | ifs
 
 // new : while loop
 while_loop: _WHILE (_conditions) _EOL (_SPACE command) (_EOL _SPACE command)* _EOL? _END_BLOCK

grammars/level17-Additions.lark

@@ -1,6 +1,6 @@
 // We add the : at the end of if and else and elif and for/while
 
-command:+= ifpressed (ifpressed_elifs)* ifpressed_elses? | ifs (elifs)* elses? -= ifs elses?
+command: += ifpressed (ifpressed_elifs)* ifpressed_elses? | ifs (elifs)* elses? -= ifs elses?
 
 // These will be handled by the preprocessor step in the merger
 // It will find the appropiate function for this anotation and modify it accordingly

grammars/level18-Additions.lark

@@ -1,6 +1,7 @@
 // adds round brackets in print() and range(), and changes ask to input
 
-command:+= input | input_empty_brackets | print_empty_brackets -= ask | error_print_no_quotes > assign | call
+command: += input | input_empty_brackets | print_empty_brackets -= ask | error_print_no_quotes >> assign | call
+_if_less_command: -= ask | error_print_no_quotes
 
 print: _PRINT (_LEFT_BRACKET (quoted_text | list_access | print_expression) (_COMMA (quoted_text | list_access | print_expression))* _RIGHT_BRACKET)?
 print_empty_brackets: _PRINT _LEFT_BRACKET _RIGHT_BRACKET

grammars/level2-Additions.lark

@@ -1,5 +1,5 @@
-//add the rules after += remove those after -= and the ones after > should be at the end
-command:+= error_ask_dep_2 | error_ask_missing_variable | assign | error_echo_dep_2 | error_non_decimal | sleep -= echo > error_invalid
+//add the rules after += remove those after -= and the ones after >> should be at the end
+command: += error_ask_dep_2 | error_ask_missing_variable | assign | error_echo_dep_2 | error_non_decimal | sleep -= echo >> error_invalid
 print: _PRINT (_print_argument)?
 _print_argument: (_SPACE | textwithoutspaces)*
 ask: var _IS _ASK ((_SPACE | text)*)?

grammars/level3-Additions.lark

@@ -1,4 +1,4 @@
-command:+= assign_list | add | remove | error_add_missing_to | error_remove_missing_from > error_invalid
+command:+= assign_list | add | remove | error_add_missing_to | error_remove_missing_from >> error_invalid
 _print_argument: (_SPACE | (list_access textwithoutspaces?) | textwithoutspaces)*
 ask: var _IS _ASK ((_SPACE | text_ask)*)?
 

grammars/level5-Additions.lark

@@ -4,7 +4,7 @@ _empty_program: (_EOL | _SPACE)*
 _non_empty_program: _EOL* (command | error_invalid) _SPACE* (_EOL+ command _SPACE*)* _EOL* //lines may end on spaces and might be separated by many newlines
 
 //placing assign after print means print is will print 'is' and print is Felienne will print 'is Felienne'
-command:+= assign_button | ifpressed_else | error_ifpressed_missing_else | ifelse | ifs -= error_invalid > assign
+command: += assign_button | ifpressed_else | error_ifpressed_missing_else | ifelse | ifs -= error_invalid >> assign
 _if_less_command: print | ask | play | turtle | assign_button | assign_list | add | remove | sleep | error_print_no_quotes | assign
 
 // error_invalid is moved from to command to program, so that command rules have priority over error_invalid

grammars/level8-Additions.lark

@@ -1,4 +1,4 @@
-command:+= error_repeat_dep_8 | error_ifelse | error_ifpressed_missing_else | ifpressed ifpressed_elses? | ifs elses?-= error_invalid_space | ifelse
+command: += error_repeat_dep_8 | error_ifelse | error_ifpressed_missing_else | ifpressed ifpressed_elses? | ifs elses? -= error_invalid_space | ifelse
 
 repeat.1: _REPEAT (INT | var_access) _TIMES _SPACE? _EOL (_SPACE command) (_EOL _SPACE command)* _EOL? _END_BLOCK
 error_repeat_dep_8: _REPEAT (INT | var_access) _TIMES _SPACE command
@@ -18,7 +18,6 @@ ifpressed_elses: elses
 //'if' cannot be used in Python, hence the name of the rule is 'ifs'
 ifs: _IF condition _EOL (_SPACE command) (_EOL _SPACE command)* _EOL? _END_BLOCK
 
-
 //old 'flat' ifelse:
-error_ifelse : ifelse
+error_ifelse : _if_clause (_SPACE+ _EOL* | _SPACE* _EOL+) _ELSE (_SPACE+ _EOL* | _SPACE* _EOL+) _if_less_command
 

grammars/level9-Additions.lark

@@ -1,3 +1,3 @@
 // only remove repeat error commands
 
-command:+= repeat | error_repeat_no_command-=repeat | error_repeat_no_command | error_repeat_no_print | error_repeat_no_times
+command: -= error_repeat_no_print | error_repeat_no_times

hedy.py

@@ -2731,7 +2731,8 @@ def merge_grammars(grammar_text_1, grammar_text_2):
     # rules that are new in the second file are added (remaining_rules_grammar_2)
     merged_grammar = []
 
-    deletables = []   # this list collects rules we no longer need,
+    deletables = []
+    # this list collects rules we no longer need,
     # they will be removed when we encounter them
 
     rules_grammar_1 = grammar_text_1.split('\n')
@@ -2765,8 +2766,7 @@ def merge_grammars(grammar_text_1, grammar_text_2):
                 if definition_1.strip() == definition_2.strip():
                     warn_message = f"The rule {name_1} is duplicated: {definition_1} and {definition_2}. Please check!"
                     warnings.warn(warn_message)
-                # Used to compute the rules that use the merge operators in the grammar
-                # namely +=, -= and >
+                # Used to compute the rules that use the merge operators in the grammar, namely +=, -= and >>
                 new_rule, new_deletables = merge_rules_operator(definition_1, definition_2, name_1, line_2_processed)
                 if new_deletables:
                     deletables += new_deletables
@@ -2791,53 +2791,55 @@ def merge_grammars(grammar_text_1, grammar_text_2):
     return '\n'.join(rules_to_keep)
 
 
-def merge_rules_operator(prev_definition, new_definition, name, complete_line):
-    # Check if the rule is adding or substracting new rules
-    has_add_op = new_definition.startswith('+=')
-    has_remove_op = has_add_op and '-=' in new_definition
-    has_last_op = has_add_op and '>' in new_definition
-    deletables = None
-    if has_remove_op:
-        # Get the rules we need to substract
-        part_list = new_definition.split('-=')
-        add_list, commands_after_minus = (part_list[0], part_list[1]) if has_remove_op else (part_list[0], '')
-        add_list = add_list[3:]
-
-        # Get the rules that need to be last
-        split_on_greater_than = commands_after_minus.split('>')
-        commands_to_be_removed, last_list = (
-            split_on_greater_than[0], split_on_greater_than[1]) if has_last_op else (split_on_greater_than[0], '')
-        commands_after_minus = commands_to_be_removed + '|' + last_list
-        result_cmd_list = get_remaining_rules(prev_definition, commands_after_minus)
-        deletables = commands_to_be_removed.strip().split('|')
-    elif has_add_op:
-        # Get the rules that need to be last
-        part_list = new_definition.split('>')
-        add_list, commands_after_minus = (part_list[0], part_list[1]) if has_last_op else (part_list[0], '')
-        add_list = add_list[3:]
-        last_list = commands_after_minus
-        result_cmd_list = get_remaining_rules(prev_definition, commands_after_minus)
-    else:
-        result_cmd_list = prev_definition
+ADD_GRAMMAR_MERGE_OP = '+='
+REMOVE_GRAMMAR_MERGE_OP = '-='
+LAST_GRAMMAR_MERGE_OP = '>>'
+GRAMMAR_MERGE_OPERATORS = [ADD_GRAMMAR_MERGE_OP, REMOVE_GRAMMAR_MERGE_OP, LAST_GRAMMAR_MERGE_OP]
 
-    if has_last_op:
-        new_rule = f"{name}: {result_cmd_list} | {add_list} | {last_list}"
-    elif has_add_op:
-        new_rule = f"{name}: {result_cmd_list} | {add_list}"
-    else:
-        new_rule = complete_line
 
-    return new_rule, deletables
+def merge_rules_operator(prev_definition, new_definition, name, complete_line):
+    op_to_arg = get_operator_to_argument(new_definition)
+
+    add_arg = op_to_arg.get(ADD_GRAMMAR_MERGE_OP, '')
+    remove_arg = op_to_arg.get(REMOVE_GRAMMAR_MERGE_OP, '')
+    last_arg = op_to_arg.get(LAST_GRAMMAR_MERGE_OP, '')
+    remaining_commands = get_remaining_rules(prev_definition, remove_arg, last_arg)
+    ordered_commands = split_rule(remaining_commands, add_arg, last_arg)
+
+    new_rule = f"{name}: {' | '.join(ordered_commands)}" if bool(op_to_arg) else complete_line
+    deletable = split_rule(remove_arg)
+    return new_rule, deletable
+
+
+def get_operator_to_argument(definition):
+    # Creates a map of all used operators and their respective arguments e.g. {'+=': 'print | play', '>>': 'echo'}
+    operator_to_index = [(op, definition.find(op)) for op in GRAMMAR_MERGE_OPERATORS if op in definition]
+    result = {}
+    for i, (op, index) in enumerate(operator_to_index):
+        start_index = index + len(op)
+        if i + 1 < len(operator_to_index):
+            _, next_index = operator_to_index[i + 1]
+            result[op] = definition[start_index:next_index].strip()
+        else:
+            result[op] = definition[start_index:].strip()
+    return result
 
 
-def get_remaining_rules(orig_def, sub_def):
-    original_commands = [command.strip() for command in orig_def.split('|')]
-    commands_after_minus = [command.strip() for command in sub_def.split('|')]
+def get_remaining_rules(orig_def, *sub_def):
+    original_commands = split_rule(orig_def)
+    commands_after_minus = split_rule(*sub_def)
+    misses = [c for c in commands_after_minus if c not in original_commands]
+    if misses:
+        raise Exception(f"Command(s) {'|'.join(misses)} do not exist in the previous definition")
     remaining_commands = [cmd for cmd in original_commands if cmd not in commands_after_minus]
     remaining_commands = ' | '.join(remaining_commands)  # turn the result list into a string
     return remaining_commands
 
 
+def split_rule(*rules):
+    return [c.strip() for rule in rules for c in rule.split('|') if c.strip() != '']
+
+
 # this is only a couple of MB in total, safe to cache
 @cache
 def create_grammar(level, lang, skip_faulty):

tests/test_grammar_merging.py

@@ -0,0 +1,102 @@
+import unittest
+from hedy import merge_rules_operator
+
+
+class TestGrammarMerging(unittest.TestCase):
+
+    def test_rule_merging_without_operators(self):
+        prev_definition = ' _PRINT (text)?'
+        new_definition = ' _PRINT (_print_argument)?'
+        complete_line = f'print {new_definition}'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'print', complete_line)
+
+        self.assertEqual(complete_line, result)
+        self.assertEqual([], deletable)
+
+    def test_rule_merging_with_greater_than_sign(self):
+        prev_definition = ' /([^\\n#ـ])([^\\n#]*)/ -> text //comment'
+        new_definition = ' /([^\\n#]+)/ -> text'
+        complete_line = 'text: /([^\\n#]+)/ -> text'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'text', complete_line)
+
+        self.assertEqual(complete_line, result)
+        self.assertEqual([], deletable)
+
+    def test_rule_merging_add(self):
+        prev_definition = 'print'
+        new_definition = '+= assign | sleep'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: print | assign | sleep', result)
+        self.assertEqual([], deletable)
+
+    def test_rule_merging_add_and_remove(self):
+        prev_definition = 'print | play | test '
+        new_definition = '+= assign | sleep -= play | print'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: test | assign | sleep', result)
+        self.assertEqual(['play', 'print'], deletable)
+
+    def test_rule_merging_add_and_last(self):
+        prev_definition = 'print | play | test '
+        new_definition = '+= assign | sleep >> play | print'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: test | assign | sleep | play | print', result)
+        self.assertEqual([], deletable)
+
+    def test_rule_merging_add_remove_and_last(self):
+        prev_definition = ' print | error | echo | test '
+        new_definition = '+= assign | sleep -= echo | print >> test | error'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', new_definition)
+
+        self.assertEqual('command: assign | sleep | test | error', result)
+        self.assertEqual(['echo', 'print'], deletable)
+
+    def test_rule_merging_remove(self):
+        prev_definition = 'print | test | assign'
+        new_definition = '-= assign | print'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: test', result)
+        self.assertEqual(['assign', 'print'], deletable)
+
+    def test_rule_merging_remove_non_existent_gives_error(self):
+        prev_definition = 'print | test | assign'
+        new_definition = '-= prin'
+
+        with self.assertRaises(Exception):
+            merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+    def test_rule_merging_remove_and_last(self):
+        prev_definition = 'play | print | test | assign'
+        new_definition = '-= assign | print >> play'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: test | play', result)
+        self.assertEqual(['assign', 'print'], deletable)
+
+    def test_rule_merging_last(self):
+        prev_definition = 'print | test | assign | play'
+        new_definition = '>> print | play'
+
+        result, deletable = merge_rules_operator(prev_definition, new_definition, 'command', '')
+
+        self.assertEqual('command: test | assign | print | play', result)
+        self.assertEqual([], deletable)
+
+    def test_rule_merging_last_non_existent_gives_error(self):
+        prev_definition = 'print | test | assign'
+        new_definition = '-= prin'
+
+        with self.assertRaises(Exception):
+            merge_rules_operator(prev_definition, new_definition, 'command', '')