miyuchina / mistletoe

A fast, extensible and spec-compliant Markdown parser in pure Python.
MIT License
791 stars 110 forks source link

fixing "pygments.util.ClassNotFound: no lexer for alias" error #183

Closed derekn closed 1 year ago

derekn commented 1 year ago

this fixes an error when an unknown language is encountered in the PygmentsRenderer

pbodnar commented 1 year ago

@derekn, thank you, LGTM. Do you think you could also add unit tests which would cover the fix (no language vs. known language vs. unknown language)? Looks like test_pygments_renderer.py is missing, so it needs to be created from scratch...

coveralls commented 1 year ago

Coverage Status

coverage: 91.086% (+0.6%) from 90.439% when pulling c0f22fc6b1f0c286242c96b47dacb82f3e9a6f7c on derekn:master into 9c218172271e4b6d9e13f4e74d7977fca0056383 on miyuchina:master.

pbodnar commented 1 year ago

Also, I think we should give clients the possibility to still get an error when an unknown language is passed. I.e. introduce a new constructor parameter, say fail_on_unknown_language=True (True for backwards compatibility), and re-throw the ClassNotFound exception if this parameter is True (and token.language is not empty).

derekn commented 1 year ago

Added unit tests for PygmentsRenderer.

Also, added the fail_on_unknown_language option to the renderer, but defaulted it to False. I believe this aligns with the behavior of other markdown packages and is generally more desirable in this situation. I dont think this will be a breaking change for clients.

pbodnar commented 1 year ago

@derekn, good point, I agree.

Just a small correction of the handling of empty language supplied (I'm not sure if I want to commit into your master branch - it is usually better to create a dedicated branch for PR, e.g. fix-pygments-classnotfound in this case):

diff --git a/mistletoe/contrib/pygments_renderer.py b/mistletoe/contrib/pygments_renderer.py
index c07a39c..0d3bd29 100644
--- a/mistletoe/contrib/pygments_renderer.py
+++ b/mistletoe/contrib/pygments_renderer.py
@@ -18,11 +18,14 @@ class PygmentsRenderer(HTMLRenderer):
     def render_block_code(self, token):
         code = token.content

-        try:
-            lexer = get_lexer(token.language)
-        except ClassNotFound as err:
-            if self.fail_on_unknown_language:
-                raise err
+        lexer = None
+        if token.language:
+            try:
+                lexer = get_lexer(token.language)
+            except ClassNotFound as err:
+                if self.fail_on_unknown_language:
+                    raise err
+        if lexer is None:
             lexer = guess_lexer(code)

         return highlight(code, lexer, self.formatter)
diff --git a/test/test_contrib/test_pygments_renderer.py b/test/test_contrib/test_pygments_renderer.py
index 491c827..839ea55 100644
--- a/test/test_contrib/test_pygments_renderer.py
+++ b/test/test_contrib/test_pygments_renderer.py
@@ -2,12 +2,17 @@ import unittest

 from mistletoe import Document
 from mistletoe.contrib.pygments_renderer import PygmentsRenderer
+from parameterized import parameterized
 from pygments.util import ClassNotFound

 class TestPygmentsRenderer(unittest.TestCase):
-    def test_render_no_language(self):
-        renderer = PygmentsRenderer()
+    @parameterized.expand([
+        (True),
+        (False)
+    ])
+    def test_render_no_language(self, fail_on_unknown_language: bool):
+        renderer = PygmentsRenderer(fail_on_unknown_language=fail_on_unknown_language)
         token = Document(['```\n', 'no language\n', '```\n'])
         output = renderer.render(token)
         actual = (

In relation to this fix, I've realized we should better name the new option fail_on_unsupported_language - can you do the rename and squash the changes to your last commit? Thank you.

derekn commented 1 year ago

@pbodnar added your changes, renamed fail_on_unsupported_language option, and squashed

derekn commented 1 year ago

Added the pygments dependency to the github action for the tests

pbodnar commented 1 year ago

Damn it, one more thing it seems - older Python builds say this:

TypeError: Parameters must be tuples, but True is not (hint: use '(True, )')

pbodnar commented 1 year ago

Added the pygments dependency to the github action for the tests

Thank you, you are quick. :) I will probably try to introduce requirements.txt in order to avoid duplication, but in a later separate commit.

pbodnar commented 1 year ago

@derekn, good job, thank you. :)

derekn commented 1 year ago

@pbodnar thanks, happy to help!