Better messaging when making incompatible changes to function signatu…

…res. (facebookincubator#7573)

Summary:
Fixes facebookincubator#7501

This is how the messaging will look when making incompatible changes:

1.  Remove a signature (`"reverse": ["(array(T)) -> array(T)"]` -> `{"reverse": []}`)
```
  Incompatible changes in function signatures have been detected.

 reverse has its function signature '(array(T)) -> array(T)' removed.

Changing or removing function signatures breaks backwards compatibility as some users may rely on function signatures that no longer exist.
```

2. Mutate a signature(`"reverse": ["(array(T)) -> array(T)"]}"` -> `"reverse": ["(array(T)) -> array(varchar)"]` )

```
  Incompatible changes in function signatures have been detected.

 'reverse(array(T)) -> array(T)' is changed to 'reverse(array(T)) -> array(varchar)'

Changing or removing function signatures breaks backwards compatibility as some users may rely on function signatures that no longer exist.
```

3. Repeat a signature(`"reverse": ["(array(T)) -> array(T)"]` -> `"reverse": ["(array(T)) -> array(T)", "(array(T)) -> array(T)"]`)

```
  Incompatible changes in function signatures have been detected.

 'reverse(array(T)) -> array(T)' is repeated 2 times.

Changing or removing function signatures breaks backwards compatibility as some users may rely on function signatures that no longer exist.
```

4. Remove a udf (`"reverse": ["(array(T)) -> array(T)"]` -> ``)

```
  Incompatible changes in function signatures have been detected.

Function 'reverse' has been removed.

Changing or removing function signatures breaks backwards compatibility as some users may rely on function signatures that no longer exist.
```

Pull Request resolved: facebookincubator#7573

Reviewed By: mbasmanova

Differential Revision: D51357044

Pulled By: kgpai

fbshipit-source-id: 54ece37c9b814bf71b521a2ad5baa31601987095

scripts/signature.py

@@ -31,6 +31,16 @@ class bcolors:
     BOLD = "\033[1m"
 
 
+def get_error_string(error_message):
+    return f"""
+Incompatible changes in function signatures have been detected.
+
+{error_message}
+
+Changing or removing function signatures breaks backwards compatibility as some users may rely on function signatures that no longer exist.
+"""
+
+
 def export(args):
     """Exports Velox function signatures."""
     pv.clear_signatures()
@@ -60,44 +70,45 @@ def diff_signatures(base_signatures, contender_signatures):
         base_signatures,
         contender_signatures,
         ignore_order=True,
+        cutoff_distance_for_pairs=0.9,
         report_repetition=True,
         view="tree",
     )
     exit_status = 0
     if delta:
         if "dictionary_item_removed" in delta:
-            print(
-                f"Signature removed: {bcolors.FAIL}{delta['dictionary_item_removed']}"
-            )
+            error_message = ""
+            for dic_removed in delta["dictionary_item_removed"]:
+                error_message += (
+                    f"""Function '{dic_removed.get_root_key()}' has been removed.\n"""
+                )
+            print(get_error_string(error_message))
             exit_status = 1
 
         if "values_changed" in delta:
-            print(f"Signature changed: {bcolors.FAIL}{delta['values_changed']}")
+            error_message = ""
+            for value_change in delta["values_changed"]:
+                error_message += f"""'{value_change.get_root_key()}{value_change.t1}' is changed to '{value_change.get_root_key()}{value_change.t2}'.\n"""
+            print(get_error_string(error_message))
             exit_status = 1
 
         if "repetition_change" in delta:
-            print(f"Signature repeated: {bcolors.FAIL}{delta['repetition_change']}")
+            error_message = ""
+            for rep_change in delta["repetition_change"]:
+                error_message += f"""'{rep_change.get_root_key()}{rep_change.t1}' is repeated {rep_change.repetition['new_repeat']} times.\n"""
+            print(get_error_string(error_message))
             exit_status = 1
 
         if "iterable_item_removed" in delta:
-            print(
-                f"Iterable item removed: {bcolors.FAIL}{delta['iterable_item_removed']}"
-            )
+            error_message = ""
+            for iter_change in delta["iterable_item_removed"]:
+                error_message += f"""{iter_change.get_root_key()} has its function signature '{iter_change.t1}' removed.\n"""
+            print(get_error_string(error_message))
             exit_status = 1
 
-        print(f"Found differences: {bcolors.OKGREEN}{delta}")
-
     else:
         print(f"{bcolors.BOLD}No differences found.")
 
-    if exit_status:
-        print(
-            f""" 
-            {bcolors.BOLD}Incompatible changes in function signatures have been detected.
-            This means your changes have modified function signatures and possibly broken backwards compatibility.  
-        """
-        )
-
     return delta, exit_status
 
 

scripts/tests/test_signature.py

@@ -13,9 +13,11 @@
 # limitations under the License.
 
 import unittest
-from scripts.signature import bias_signatures
+import unittest.mock
+from scripts.signature import bias_signatures, get_error_string
 from pathlib import Path
 import json
+import io
 
 
 def read_from_file(file_path):
@@ -65,6 +67,54 @@ def test_bias(self):
 
         self.assertEqual(bias_functions, "bar=10,foo=10")
 
+    @unittest.mock.patch("sys.stdout", new_callable=io.StringIO)
+    def get_bias_messaging(self, base_signatures, contender_signatures, mock_stdout):
+        test_bias(base_signatures, contender_signatures)
+        return mock_stdout.getvalue()
+
+    def assert_messaging(self, base_signatures, contender_signatures, expected_message):
+        test_bias(base_signatures, contender_signatures)
+        actual = self.get_bias_messaging(base_signatures, contender_signatures)
+        expected = get_error_string(expected_message)
+        expected += "\n"  # Add trailing newline for std output.
+        self.assertEquals(expected, actual)
+
+    def test_messaging(self):
+        # Remove a signature
+        self.assert_messaging(
+            """{"reverse": ["(array(T)) -> array(T)"]}""",
+            """{"reverse": []}""",
+            "reverse has its function signature '(array(T)) -> array(T)' removed.\n",
+        )
+
+        # Remove more than one signature
+        self.assert_messaging(
+            """{"reverse": ["(array(T)) -> array(T)", "(varchar) -> varchar"]}""",
+            """{"reverse": []}""",
+            """reverse has its function signature '(array(T)) -> array(T)' removed.\nreverse has its function signature '(varchar) -> varchar' removed.\n""",
+        )
+
+        # Mutate a signature
+        self.assert_messaging(
+            """{"reverse": ["(array(T)) -> array(T)"]}""",
+            """{"reverse": ["(array(T)) -> array(varchar)"]}""",
+            """'reverse(array(T)) -> array(T)' is changed to 'reverse(array(T)) -> array(varchar)'.\n""",
+        )
+
+        # Function repeated
+        self.assert_messaging(
+            """{"reverse": ["(array(T)) -> array(T)"]}""",
+            """{"reverse": ["(array(T)) -> array(T)", "(array(T)) -> array(T)"]}""",
+            "'reverse(array(T)) -> array(T)' is repeated 2 times.\n",
+        )
+
+        # Remove a udf
+        self.assert_messaging(
+            """{"reverse": ["(array(T)) -> array(T)"]}""",
+            """{}""",
+            "Function 'reverse' has been removed.\n",
+        )
+
 
 if __name__ == "__main__":
     unittest.main()