Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading error/exception thrown with invalid data . #147

Closed
rgammans opened this issue Jan 23, 2021 · 4 comments · Fixed by #150
Closed

Misleading error/exception thrown with invalid data . #147

rgammans opened this issue Jan 23, 2021 · 4 comments · Fixed by #150

Comments

@rgammans
Copy link
Contributor

When unexpected errors occur in the markdownify, or function which rely on it the martor.utils.VersionNotCompatible, exception
is thrown rather than propagating the original exception. I generally feel that pattern try-except in markdownify eg..

  try:
     .
     .
 except Exception:
   .

has too broad a 'catch' clause.

Can you consider narrowing the clause?

Details

  • Browser and browser version: No Browser, server-side issue.
  • OS version: Debian 10.
  • Martor version: 1.5.8

Steps to reproduce

(minimal example)

  1. Start a clean directory.
  2. pipenv install martor
  3. create settings.py
SECRET_KEY="qaswejuiokl" # anything to make django happy.
  1. create test.py
import os
os.environ['DJANGO_SETTINGS_MODULE']='settings'
import martor.utils as utils

print(utils.markdownify(None))
  1. pipenv run python test.py
@rgammans
Copy link
Contributor Author

I suggest a patch along these lines

--- a/martor/utils.py
+++ b/martor/utils.py
@@ -33,7 +33,9 @@ def markdownify(markdown_content):
             extensions=MARTOR_MARKDOWN_EXTENSIONS,
             extension_configs=MARTOR_MARKDOWN_EXTENSION_CONFIGS
         )
-    except Exception:
+    except TypeError as e:
+        if 'extendMarkdown' not in str(e):
+            raise
         raise VersionNotCompatible("The markdown isn't compatible, please reinstall "
                                    "your python markdown into Markdown>=3.0")

@agusmakmun
Copy link
Owner

@rgammans oh ok, nice one. can you please create new P.R?

@rgammans
Copy link
Contributor Author

rgammans commented Jan 25, 2021

Ok, I can do this. Do I need to add a unit test? I feel I should but there doesn't seem to be very many in this project as it stands at the moment

@agusmakmun
Copy link
Owner

@rgammans no need unitest is no problem.

rgammans added a commit to rgammans/django-markdown-editor that referenced this issue Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants