Repository: Python-Markdown/markdown
Pull request #939 (2020-04-14 08:02:29)

extend_markdown() TypeErrors swallowed because of deprecation test.
Currently, an extension's `extendMarkdown()` raising a TypeError will be swallowed, or not reported.  This occurs because the calling `_extendMarkdown()` assumes the TypeError occurs because an `md_globals` argument was also passed.  This pull request modifies `_extendMarkdown()` to check this assumption.

Also, trying to delete an invalid key is a KeyError as opposed to a TypeError.

Have a wonderful day!


mitya57: Tests in `tests/test_apis.py` should be changed to expect KeyError instead of TypeError.

I hope none of third-party extensions relied on this wrong exception type…
merriam: Updated unit test
merriam: While nothing in the repository uses this error outside of unit tests, it cannot be proven that some project, somewhere, relies upon it.  It would be a stretch to imagine why.

That said, I found a bug.  Fixed it.  Went back and fixed the buggy unit tests.   I'm OK with it never being adopted but not OK writing more makework tests.  It's a bad practice, and, if we ever meet for beers, I can give twenty years of research on it.

Other fish to fry...

waylan: This should get a comment in the [release notes][1] under version 3.2.2.

[1]. https://github.com/Python-Markdown/markdown/blob/master/docs/change_log/index.md
merriam: Er, Travis fails because it doesn't know KeyError and TypeError spelling?
mitya57: @merriam As Waylan noticed, you need to put the class names into backticks: `KeyError` → `` `KeyError` ``.
waylan: > Er, Travis fails because it doesn't know KeyError and TypeError spelling?

You don't expect KeyError and TypeError to be in the English dictionary do you? Sure, they are known Python words, but then they are code and the spellchecker ignores code. So, put then in code spans where they belong.

Tests accomplish two purposes: (1) ensure we don't break our code and (2) ensure we follow our own style guide. This accomplishes the later.
merriam: Sorry, I'm giving up.   

The code changes took about 20 minutes.  Getting it accepted has taken several hours, and I have no idea how many more steps will appear.   It may a joy for some, though my way is more on the development side.

The current state is that any TypeError thrown by an `extendMarkdown()` will cause a secondary error when it is called again with too many parameters.
waylan: Our process and requirements for PRs are all documented in our [Contributing Guide][1]. If we were to ignore those requirements our code and documentation would be a mess. Not sure how that would be better for anyone.

[1]: https://python-markdown.github.io/contributing/
waylan: Oh and I would have happily made a few edits to fix the "spelling errors." However, you put the notes in the wrong file. To fix them I need to clone your fork locally, make the changes and push the changes. If you had put them in the correct location, I could have simply used GitHub's web interface to add a few backticks. I even linked to the correct location when I asked for the release notes.

Patch:
diff --git a/docs/change_log/index.md b/docs/change_log/index.md
index 8f1b9db..121dab0 100644
--- a/docs/change_log/index.md
+++ b/docs/change_log/index.md
@@ -6,7 +6,10 @@ Python-Markdown Change Log
 Under development: version 3.2.2 (a bug-fix release).
 
 * Load entry_points (for extensions) only once using `importlib.metadata`.
-* Fixed issue where double escaped entities could end up in TOC.
+* Do not double escape entities in TOC.
+* Correctly report if an extension raises a `TypeError` (#939).
+* Raise a `KeyError` when attempting to delete a nonexistent key from the
+  extension registry (#939). 
 
 Feb 12, 2020: Released version 3.2.1 (a bug-fix release).
 
diff --git a/markdown/extensions/__init__.py b/markdown/extensions/__init__.py
index 010e310..4bc8e5f 100644
--- a/markdown/extensions/__init__.py
+++ b/markdown/extensions/__init__.py
@@ -75,15 +75,18 @@ class Extension:
         md = args[0]
         try:
             self.extendMarkdown(md)
-        except TypeError:
-            # Must be a 2.x extension. Pass in a dumby md_globals.
-            self.extendMarkdown(md, {})
-            warnings.warn(
-                "The 'md_globals' parameter of '{}.{}.extendMarkdown' is "
-                "deprecated.".format(self.__class__.__module__, self.__class__.__name__),
-                category=DeprecationWarning,
-                stacklevel=2
-            )
+        except TypeError as e:
+            if "missing 1 required positional argument" in str(e):
+                # Must be a 2.x extension. Pass in a dumby md_globals.
+                self.extendMarkdown(md, {})
+                warnings.warn(
+                    "The 'md_globals' parameter of '{}.{}.extendMarkdown' is "
+                    "deprecated.".format(self.__class__.__module__, self.__class__.__name__),
+                    category=DeprecationWarning,
+                    stacklevel=2
+                )
+            else:
+                raise
 
     def extendMarkdown(self, md):
         """
diff --git a/markdown/util.py b/markdown/util.py
index 056fd72..a8db7bd 100644
--- a/markdown/util.py
+++ b/markdown/util.py
@@ -399,7 +399,7 @@ class Registry:
                 stacklevel=2,
             )
         else:
-            raise TypeError
+            raise KeyError('Cannot delete key {}, not registered.'.format(key))
 
     def add(self, key, value, location):
         """ Register a key by location. """


Test patch:
diff --git a/tests/test_apis.py b/tests/test_apis.py
index 39236f2..6564c66 100644
--- a/tests/test_apis.py
+++ b/tests/test_apis.py
@@ -337,7 +337,7 @@ class RegistryTests(unittest.TestCase):
     def testRegistryDelItem(self):
         r = markdown.util.Registry()
         r.register(Item('a'), 'a', 20)
-        with self.assertRaises(TypeError):
+        with self.assertRaises(KeyError):
             del r[0]
         # TODO: restore this when deprecated __del__ is removed.
         # with self.assertRaises(TypeError):
@@ -352,7 +352,7 @@ class RegistryTests(unittest.TestCase):
             self.assertEqual(list(r), ['a', 'c'])
             del r['a']
             self.assertEqual(list(r), ['c'])
-            with self.assertRaises(TypeError):
+            with self.assertRaises(KeyError):
                 del r['badname']
             del r['c']
             self.assertEqual(list(r), [])