From 8713078e07defdf913377408ff5c4085a3bbaf03 Mon Sep 17 00:00:00 2001
From: Juan Mauricio Matera <matera@fisica.unlp.edu.ar>
Date: Fri, 4 Nov 2022 10:16:39 -0300
Subject: [PATCH] my suggestions (#592)

---
 mathics/builtin/assignments/internals.py | 58 ++++++++++++------------
 mathics/builtin/base.py                  | 16 +++----
 2 files changed, 36 insertions(+), 38 deletions(-)

diff --git a/mathics/builtin/assignments/internals.py b/mathics/builtin/assignments/internals.py
index 26a7bb42c..de67bd3a4 100644
--- a/mathics/builtin/assignments/internals.py
+++ b/mathics/builtin/assignments/internals.py
@@ -558,6 +558,21 @@ def process_assign_format(self, lhs, rhs, evaluation, tags, upset):
     return count > 0
 
 
+def process_assign_makeboxes(self, lhs, rhs, evaluation, tags, upset):
+    # FIXME: the below is a big hack.
+    # Currently MakeBoxes boxing is implemented as a bunch of rules.
+    # See mathics.builtin.base contribute().
+    # I think we want to change this so it works like normal SetDelayed
+    # That is:
+    #   MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
+    # rather than:
+    #   MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
+    makeboxes_rule = Rule(lhs, rhs, system=True)
+    makeboxes_defs = evaluation.definitions.builtin["System`MakeBoxes"]
+    makeboxes_defs.add_rule(makeboxes_rule)
+    return True
+
+
 def process_assign_messagename(self, lhs, rhs, evaluation, tags, upset):
     lhs, condition = unroll_conditions(lhs)
     lhs, rhs = unroll_patterns(lhs, rhs, evaluation)
@@ -685,23 +700,24 @@ def process_tags_and_upset_allow_custom(tags, upset, self, lhs, evaluation):
 
 class _SetOperator:
     special_cases = {
-        "System`OwnValues": process_assign_definition_values,
-        "System`DownValues": process_assign_definition_values,
-        "System`SubValues": process_assign_definition_values,
-        "System`UpValues": process_assign_definition_values,
-        "System`NValues": process_assign_definition_values,
-        "System`DefaultValues": process_assign_definition_values,
-        "System`Messages": process_assign_definition_values,
-        "System`Attributes": process_assign_attributes,
-        "System`Options": process_assign_options,
-        "System`$RandomState": process_assign_random_state,
         "System`$Context": process_assign_context,
         "System`$ContextPath": process_assign_context_path,
-        "System`N": process_assign_n,
-        "System`NumericQ": process_assign_numericq,
-        "System`MessageName": process_assign_messagename,
+        "System`$RandomState": process_assign_random_state,
+        "System`Attributes": process_assign_attributes,
         "System`Default": process_assign_default,
+        "System`DefaultValues": process_assign_definition_values,
+        "System`DownValues": process_assign_definition_values,
         "System`Format": process_assign_format,
+        "System`MakeBoxes": process_assign_makeboxes,
+        "System`MessageName": process_assign_messagename,
+        "System`Messages": process_assign_definition_values,
+        "System`N": process_assign_n,
+        "System`NValues": process_assign_definition_values,
+        "System`NumericQ": process_assign_numericq,
+        "System`Options": process_assign_options,
+        "System`OwnValues": process_assign_definition_values,
+        "System`SubValues": process_assign_definition_values,
+        "System`UpValues": process_assign_definition_values,
     }
 
     def assign_elementary(self, lhs, rhs, evaluation, tags=None, upset=False):
@@ -755,21 +771,5 @@ def assign(self, lhs, rhs, evaluation):
                 return False
             indices = lhs.elements[1:]
             return walk_parts([rule.replace], indices, evaluation, rhs)
-
-        # FIXME: the below is a big hack.
-        # Currently MakeBoxes boxing is implemented as a bunch of rules.
-        # See mathics.builtin.base contribute().
-        # I think we want to change this so it works like normal SetDelayed
-        # That is:
-        #   MakeBoxes[CubeRoot, StandardForm] := RadicalBox[3, StandardForm]
-        # rather than:
-        #   MakeBoxes[CubeRoot, StandardForm] -> RadicalBox[3, StandardForm]
-        elif lhs.get_head_name() == "System`MakeBoxes":
-            makeboxes_rule = Rule(lhs, rhs, system=True)
-            makeboxes_defs = defs.builtin["System`MakeBoxes"]
-            makeboxes_defs.add_rule(makeboxes_rule)
-            # FIXME: what should be the result?
-            return makeboxes_rule
-
         else:
             return self.assign_elementary(lhs, rhs, evaluation)
diff --git a/mathics/builtin/base.py b/mathics/builtin/base.py
index 10994f721..b6faf3aac 100644
--- a/mathics/builtin/base.py
+++ b/mathics/builtin/base.py
@@ -283,15 +283,13 @@ def check_options(options_to_check, evaluation):
             rules.append(
                 BuiltinRule(name, pattern, function, check_options, system=True)
             )
-        for pattern, replace in self.rules.items():
-            # FIXME: sometimes pattern is a string and sometimes a BaseElement?
-            # This seems wrong.
-            if not isinstance(pattern, BaseElement):
-                pattern = pattern % {"name": name}
-                pattern = parse_builtin_rule(pattern, definition_class)
-            replace = replace % {"name": name}
-            # FIXME: Should system=True be system=not is_pymodule ?
-            rules.append(Rule(pattern, parse_builtin_rule(replace), system=True))
+        for pattern_str, replace_str in self.rules.items():
+            pattern_str = pattern_str % {"name": name}
+            pattern = parse_builtin_rule(pattern_str, definition_class)
+            replace_str = replace_str % {"name": name}
+            rules.append(
+                Rule(pattern, parse_builtin_rule(replace_str), system=not is_pymodule)
+            )
 
         box_rules = []
         # FIXME: Why a special case for System`MakeBoxes? Remove this