Repository: exercism/python
Pull request #1809 (2019-06-01 06:23:58)

sublists: sync with canonical data
part of  #1762 

- renamed the function and arguments in stub, tests and example.
- deleted references to `SUBLIST` `SUPERLIST` `EQUAL` `UNEQUAL` as [canonical_data.json](https://github.com/exercism/problem-specifications/blob/master/exercises/sublist/canonical-data.json) seems to want a string result.
- deleted a test that referenced those variables only.
- made the additional tests more consistent with the indentation scheme of the test file.

Grociu: Thank you @yawpitch for this write-up and the time you took in making this post. I've learned something new, which is always appreciated.

For the issue at hand, I can rewrite this to include these global constants, but would that be considered consistent with canonical data (by the maintainer), as per the definition of the task? We've made several minor compromises with the other examples already but this seems more major. The 'yacht' exercise uses a similar scheme of NAME variables so that would clarify further syncs as well.

In addition, while these exercises (sublists, yacht, maybe others?) use the aforementioned scheme, it's not explained in any way. It can be somewhat inferred from the test function, but it causes confusion - I did yacht quite early into learning Python, and I was confused. Is there any interest of putting a small explanation/resource of that into the `README.md` maybe?
 
cmccandless: > For the issue at hand, I can rewrite this to include these global constants, but would that be considered consistent with canonical data (by the maintainer), as per the definition of the task?

Yes. Synchronization with the canonical data is not helpful if the tests become un-[Pythonic](https://blog.startifact.com/posts/older/what-is-pythonic.html).

> In addition, while these exercises (sublists, yacht, maybe others?) use the aforementioned scheme, it's not explained in any way. It can be somewhat inferred from the test function, but it causes confusion - I did yacht quite early into learning Python, and I was confused. Is there any interest of putting a small explanation/resource of that into the README.md 

Whichever exercise in the track progression first uses returned constants should contain something in its HINTS.md mentioning this. Perhaps exercise stubs should also contain stub entries for the enumeration items?

```Python
SUBLIST = None
SUPERLIST = None
EQUAL = None
```

This exercise currently contains a check to make sure these values are unique; this should probably be done for all exercises that use returned constants.

Additionally, it may be worthwhile to mention this in our [List of Conventions](https://github.com/exercism/python/blob/master/README.md#conventions)
yawpitch: Struggling to maintain strong parity with the canonical data is not a worthwhile endeavor; the canonical data writers cannot know what abstractions exist in all the track languages, and it’s going to have to be up to the track’s task implementer to determine if literal datatype in the canonical version is a good Python idiom. From this point of view deviating from the canonical data _to teach the language better_ is not a compromise... slavishly following the canonical data, though, _would_ be a compromise.

Enumerated constants are a basic programming pattern and students need to get used to seeing them, so we certainly should use them in problems in which they’re appropriate.

Any time the tests **must** have access to a constant value that the thing under test either requires as an argument or returns as a value then these global constants are a sensible and idiomatic design choice.

Now, in Python 3 there’s the **enum** module, and ideally we could use that instead of raw global names, but there’s an unknown amount of time until we’re able or willing to abandon Python 2 support.

And yes, updating the README is a reasonable first step if it helps reduce confusion. Personally I think their existence in the “slug” file they get when they download the exercise is sufficient, but for beginners it may not be.


cmccandless: > there’s an unknown amount of time until we’re able or willing to abandon Python 2 support.

Just to make a note here on that: I believe all issues related to [dropping Python 2](https://github.com/exercism/python/milestone/1) have been closed, so those changes will probably be merged sometime in the second half of this year.
yawpitch: Good to hear. If we’re assuming 3.6+ we can do a lot to improve things for yourselves and for students. Actual enum types, type hinting, and f-strings... though we should be careful too heavily promoting the latter, as they’re still impervious to translation into other languages.
On Jun 18, 2019, 19:24 +0100, Corey McCandless <notifications@github.com>, wrote:
> > there’s an unknown amount of time until we’re able or willing to abandon Python 2 support.
> Just to make a note here on that: I believe all issues related to dropping Python 2 have been closed, so those changes will probably be merged sometime in the second half of this year.
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

cmccandless: > f-strings... though we should be careful too heavily promoting the latter, as they’re still impervious to translation into other languages.

Not really; string interpolation is present in a lot of common languages these days: C#, Javascript, and Ruby all have it, although syntax varies a little.
yawpitch: Sorry, should have been more clear: the Python f-string is not well-suited for internationalization / localization... an f-string that starts in English will remain in English, while template strings that use **str.format** are amenable to translation with standard tools like `gettext`. I was not referring to string interpolation or translation into other programming languages.
Grociu: Second revision:

- Returned the enumerated constants to the exercise test suite.
- Entered the enumerated constants into the exercise stub, with `None` as the base value.
- Returned the uniqueness test into the test suite, and moved to the end with other tests not included in [canonical-data.json](https://github.com/exercism/problem-specifications/blob/master/exercises/sublist/canonical-data.json)
- Minor renaming of variables in one of the tests
- Added a HINTS.md file with a basic explanation of the concept based on feedback from @yawpitch 

yawpitch: If I'm not mistaken, at this point -- after my comment about HINTS.md is taken into account -- the only _substantive_ change is renaming **check_lists** to **sublist**, correct? The rest is just formatting and a few temporary variable names?

For @cmccandless: is syncing the name of the function under test to the canonical data that important? I ask only because this PR will "break" every currently published Community Solution for the exercise without adding any actual new tests.
Grociu: Third pass:

- Deleted the hint file, and moved the contents to the exercise stub.
yawpitch: Ok, notwithstanding my concern about invalidating all the existing Community Solutions for the dubious value of having the function name be the same as the one on the canonical data, this is safe to merge without further changes.

I personally don’t think the name being in sync is worth it, nor something most tracks would fight for. The odds of all properties in the canonical data files being named in a manner consistent with good practice in any language are _low_.

But I’ll leave that determination to @cmccandless.

Patch:
diff --git a/exercises/sublist/example.py b/exercises/sublist/example.py
index f7d7f25d..94833778 100644
--- a/exercises/sublist/example.py
+++ b/exercises/sublist/example.py
@@ -4,26 +4,26 @@ EQUAL = 2
 UNEQUAL = 3
 
 
-def check_lists(l1, l2):
-    if l1 == l2:
+def sublist(list_one, list_two):
+    if list_one == list_two:
         return EQUAL
-    if contains(l1, l2):
+    if contains(list_one, list_two):
         return SUPERLIST
-    if contains(l2, l1):
+    if contains(list_two, list_one):
         return SUBLIST
     return UNEQUAL
 
 
-def contains(l1, l2):
-    if not l2:
+def contains(list_one, list_two):
+    if not list_two:
         return True
-    if len(l2) > len(l1):
+    if len(list_two) > len(list_one):
         return False
-    for i in range(len(l1) - len(l2) + 1):
-        if l1[i] != l2[0]:
+    for i in range(len(list_one) - len(list_two) + 1):
+        if list_one[i] != list_two[0]:
             continue
-        for j in range(len(l2)):
-            if l1[i + j] != l2[j]:
+        for j in range(len(list_two)):
+            if list_one[i + j] != list_two[j]:
                 break
         else:
             return True
diff --git a/exercises/sublist/sublist.py b/exercises/sublist/sublist.py
index 0c7b0037..428d3aa6 100644
--- a/exercises/sublist/sublist.py
+++ b/exercises/sublist/sublist.py
@@ -1,2 +1,23 @@
-def check_lists(first_list, second_list):
+"""
+This exercise stub and the test suite contain several enumerated constants.
+
+Since Python 2 does not have the enum module, the idiomatic way to write
+enumerated constants has traditionally been a NAME assigned to an arbitrary,
+but unique value. An integer is traditionally used because it’s memory
+efficient.
+It is a common practice to export both constants and functions that work with
+those constants (ex. the constants in the os, subprocess and re modules).
+
+You can learn more here: https://en.wikipedia.org/wiki/Enumerated_type
+"""
+
+# Possible sublist categories.
+# Change the values as you see fit.
+SUBLIST = None
+SUPERLIST = None
+EQUAL = None
+UNEQUAL = None
+
+
+def sublist(list_one, list_two):
     pass


Test patch:
diff --git a/exercises/sublist/sublist_test.py b/exercises/sublist/sublist_test.py
index 9bab3842..6bc21dfa 100644
--- a/exercises/sublist/sublist_test.py
+++ b/exercises/sublist/sublist_test.py
@@ -1,134 +1,141 @@
 import unittest
 
-from sublist import check_lists, SUBLIST, SUPERLIST, EQUAL, UNEQUAL
+from sublist import sublist, SUBLIST, SUPERLIST, EQUAL, UNEQUAL
 
 
 # Tests adapted from `problem-specifications//canonical-data.json` @ v1.1.0
 
 class SublistTest(unittest.TestCase):
-    def test_unique_return_values(self):
-        self.assertEqual(len(set([SUBLIST, SUPERLIST, EQUAL, UNEQUAL])), 4)
-
     def test_empty_lists(self):
         self.assertEqual(
-            check_lists([], []),
+            sublist([], []),
             EQUAL
         )
 
     def test_empty_list_within_non_empty_list(self):
         self.assertEqual(
-            check_lists([], [1, 2, 3]),
+            sublist([], [1, 2, 3]),
             SUBLIST
         )
 
     def test_non_empty_list_contains_empty_list(self):
         self.assertEqual(
-            check_lists([1, 2, 3], []),
+            sublist([1, 2, 3], []),
             SUPERLIST
         )
 
     def test_list_equals_itself(self):
         self.assertEqual(
-            check_lists([1, 2, 3], [1, 2, 3]),
+            sublist([1, 2, 3], [1, 2, 3]),
             EQUAL
         )
 
     def test_different_lists(self):
         self.assertEqual(
-            check_lists([1, 2, 3], [2, 3, 4]),
+            sublist([1, 2, 3], [2, 3, 4]),
             UNEQUAL
         )
 
     def test_false_start(self):
         self.assertEqual(
-            check_lists([1, 2, 5], [0, 1, 2, 3, 1, 2, 5, 6]),
+            sublist([1, 2, 5], [0, 1, 2, 3, 1, 2, 5, 6]),
             SUBLIST
         )
 
     def test_consecutive(self):
         self.assertEqual(
-            check_lists([1, 1, 2], [0, 1, 1, 1, 2, 1, 2]),
+            sublist([1, 1, 2], [0, 1, 1, 1, 2, 1, 2]),
             SUBLIST
         )
 
     def test_sublist_at_start(self):
         self.assertEqual(
-            check_lists([0, 1, 2], [0, 1, 2, 3, 4, 5]),
+            sublist([0, 1, 2], [0, 1, 2, 3, 4, 5]),
             SUBLIST
         )
 
     def test_sublist_in_middle(self):
         self.assertEqual(
-            check_lists([2, 3, 4], [0, 1, 2, 3, 4, 5]),
+            sublist([2, 3, 4], [0, 1, 2, 3, 4, 5]),
             SUBLIST
         )
 
     def test_sublist_at_end(self):
         self.assertEqual(
-            check_lists([3, 4, 5], [0, 1, 2, 3, 4, 5]),
+            sublist([3, 4, 5], [0, 1, 2, 3, 4, 5]),
             SUBLIST
         )
 
     def test_at_start_of_superlist(self):
         self.assertEqual(
-            check_lists([0, 1, 2, 3, 4, 5], [0, 1, 2]),
+            sublist([0, 1, 2, 3, 4, 5], [0, 1, 2]),
             SUPERLIST
         )
 
     def test_in_middle_of_superlist(self):
         self.assertEqual(
-            check_lists([0, 1, 2, 3, 4, 5], [2, 3]),
+            sublist([0, 1, 2, 3, 4, 5], [2, 3]),
             SUPERLIST
         )
 
     def test_at_end_of_superlist(self):
         self.assertEqual(
-            check_lists([0, 1, 2, 3, 4, 5], [3, 4, 5]),
+            sublist([0, 1, 2, 3, 4, 5], [3, 4, 5]),
             SUPERLIST
         )
 
     def test_first_list_missing_element_from_second_list(self):
         self.assertEqual(
-            check_lists([1, 3], [1, 2, 3]),
+            sublist([1, 3], [1, 2, 3]),
             UNEQUAL
         )
 
     def test_second_list_missing_element_from_first_list(self):
         self.assertEqual(
-            check_lists([1, 2, 3], [1, 3]),
+            sublist([1, 2, 3], [1, 3]),
             UNEQUAL
         )
 
     def test_order_matters_to_a_list(self):
         self.assertEqual(
-            check_lists([1, 2, 3], [3, 2, 1]),
+            sublist([1, 2, 3], [3, 2, 1]),
             UNEQUAL
         )
 
     def test_same_digits_but_different_numbers(self):
         self.assertEqual(
-            check_lists([1, 0, 1], [10, 1]),
+            sublist([1, 0, 1], [10, 1]),
             UNEQUAL
         )
 
+    # additional track specific test
+    def test_unique_return_values(self):
+        self.assertEqual(len(set([SUBLIST, SUPERLIST, EQUAL, UNEQUAL])), 4)
+
     # additional track specific test
     def test_inner_spaces(self):
         self.assertEqual(
-            check_lists(['a c'], ['a', 'c']),
+            sublist(['a c'], ['a', 'c']),
             UNEQUAL
         )
 
     # additional track specific test
     def test_large_lists(self):
-        l1 = list(range(1000)) * 1000 + list(range(1000, 1100))
-        l2 = list(range(900, 1050))
-        self.assertEqual(check_lists(l1, l2), SUPERLIST)
+        list_1 = list(range(1000)) * 1000 + list(range(1000, 1100))
+        list_2 = list(range(900, 1050))
+        self.assertEqual(
+            sublist(list_1, list_2),
+            SUPERLIST
+        )
 
     # additional track specific test
     def test_spread_sublist(self):
         multiples_of_3 = list(range(3, 200, 3))
         multiples_of_15 = list(range(15, 200, 15))
-        self.assertEqual(check_lists(multiples_of_15, multiples_of_3), UNEQUAL)
+        self.assertEqual(
+            sublist(multiples_of_15, multiples_of_3),
+            UNEQUAL
+        )
 
 
 if __name__ == '__main__':