✨ NEW: Add linkify #78

tsutsu3 · 2020-11-15T11:44:55Z

close #5

Implement linkify with linkify-it-py

Implementation memo

~~If likify-it-py is not installed, import dummy LinkifyIt~~
Throw an exception only when linkify rule enabled, linkify option is True and likify-it-py is not installed
The exceptions raise at parse time, not at initialization time
Avoided including likify-it-py in dependency package

…ages

codecov · 2020-11-15T11:46:53Z

Codecov Report

Merging #78 (994c0ad) into master (aa54b8b) will increase coverage by 0.02%.
The diff coverage is 95.78%.

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
+ Coverage   95.64%   95.67%   +0.02%     
==========================================
  Files          76       77       +1     
  Lines        3813     3907      +94     
==========================================
+ Hits         3647     3738      +91     
- Misses        166      169       +3

Flag	Coverage Δ
pytests	`95.67% <95.78%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
markdown_it/main.py	`90.98% <60.00%> (-1.33%)`	⬇️
markdown_it/rules_core/linkify.py	`97.72% <97.72%> (ø)`
markdown_it/parser_core.py	`100.00% <100.00%> (ø)`
markdown_it/rules_core/__init__.py	`100.00% <100.00%> (ø)`
markdown_it/common/utils.py	`71.60% <0.00%> (+1.23%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa54b8b...994c0ad. Read the comment docs.

chrisjsewell · 2020-11-15T11:56:30Z

The final piece of the puzzle!
Thanks 🙏 I should be able to have a look later today

tsutsu3 · 2020-11-15T11:58:56Z

Is this implementation good?

If it's OK, increase coverage

chrisjsewell · 2020-11-15T12:20:43Z

Well it passes all the markdown-it fixture tests. So that's always a good start 😀

@hukkinj1 might also want to have a look?

hukkin

Nice! I would suggest a change in how the "linkify enabled but not installed" case is handled, that should result in overall a bit less code, and avoids giving LinkifyIt.pretest a secondary task.

hukkin · 2020-11-15T15:06:05Z

markdown_it/extra/__init__.py

@@ -0,0 +1 @@
+from .linkify_it import LinkifyIt  # noqa: F401


Suggested change

from .linkify_it import LinkifyIt # noqa: F401

hukkin · 2020-11-15T15:08:06Z

markdown_it/extra/linkify_it.py

+class NotImportedError(Exception):
+    """Not imported ``linkify-it-py`` package error"""
+
+    def __init__(self):
+        message = (
+            "If you want to use the 'linkify', "
+            + "you must pre-install the 'linkify-it-py' package. "
+            + "Please try 'pip install linkify-it-py'."
+        )
+        super().__init__(message)
+
+
+class LinkifyIt:
+    """linkify-it-py dummy class.
+
+    If ``linkify-it-py`` is not installed, this dummy class will be called. And
+    raise :class:`markdown_it.extra.linkify_it.NotImportedError` when input data
+    is parsed via core.rules (linkify).
+    """
+
+    def __init__(self, schemas=None, options=None):
+        pass
+
+    # def add(self, schema, definition):
+    #     raise NotImportedError
+
+    # def match(self, text):
+    #     raise NotImportedError
+
+    def pretest(self, text):
+        raise NotImportedError
+
+    # def set(self, options):
+    #     raise NotImportedError
+
+    # def test(self, text):
+    #     raise NotImportedError
+
+    # def test_schema_at(self, text, name, position):
+    #     raise NotImportedError
+
+    # def tlds(self, list_tlds, keep_old=False):
+    #     raise NotImportedError


Suggested change

class NotImportedError(Exception):

"""Not imported ``linkify-it-py`` package error"""

def __init__(self):

message = (

"If you want to use the 'linkify', "

+ "you must pre-install the 'linkify-it-py' package. "

+ "Please try 'pip install linkify-it-py'."

)

super().__init__(message)

class LinkifyIt:

"""linkify-it-py dummy class.

If ``linkify-it-py`` is not installed, this dummy class will be called. And

raise :class:`markdown_it.extra.linkify_it.NotImportedError` when input data

is parsed via core.rules (linkify).

"""

def __init__(self, schemas=None, options=None):

pass

# def add(self, schema, definition):

# raise NotImportedError

# def match(self, text):

# raise NotImportedError

def pretest(self, text):

raise NotImportedError

# def set(self, options):

# raise NotImportedError

# def test(self, text):

# raise NotImportedError

# def test_schema_at(self, text, name, position):

# raise NotImportedError

# def tlds(self, list_tlds, keep_old=False):

# raise NotImportedError

hukkin · 2020-11-15T15:09:26Z

markdown_it/main.py

+try:
+    from linkify_it import LinkifyIt
+except (ImportError, ModuleNotFoundError):
+    from markdown_it.extra import LinkifyIt


Suggested change

try:

from linkify_it import LinkifyIt

except (ImportError, ModuleNotFoundError):

from markdown_it.extra import LinkifyIt

try:

import linkify_it

except ModuleNotFoundError:

linkify_it = None

hukkin · 2020-11-15T15:12:30Z

markdown_it/main.py

@@ -41,8 +46,7 @@ def __init__(
        self.options = {}
        self.configure(config)

-        # var LinkifyIt    = require('linkify-it')
-        # self.linkify = LinkifyIt()  # TODO maybe see https://github.com/Suor/autolink
+        self.linkify = LinkifyIt()


Suggested change

self.linkify = LinkifyIt()

self.linkify = linkify_it.LinkifyIt() if linkify_it else None

markdown_it/rules_core/linkify.py

tsutsu3 · 2020-11-16T11:54:54Z

Nice! I would suggest a change in how the "linkify enabled but not installed" case is handled, that should result in overall a bit less code, and avoids giving LinkifyIt.pretest a secondary task.

Oh, thanks. 😄

markdown_it/rules_core/linkify.py

hukkin · 2020-11-17T15:31:05Z

markdown_it/rules_core/linkify.py

+        # We scan from the end, to keep position when new tags added.
+        # Use reversed logic in links start/end match
+        assert tokens is not None
+        for i in range(len(tokens))[::-1]:


This is just a preference thing but I personally always found reversed(iterable) more readable than iterable[::-1]

for statement did not calculate the index i correctly.

So I changed to a while statement.

4a97b09

hukkin · 2020-11-17T15:34:10Z

markdown_it/rules_core/linkify.py

+        return
+
+    if not state.md.linkify:
+        raise Exception("Linkify enabed but not installed.")


Suggested change

raise Exception("Linkify enabed but not installed.")

raise Exception("Linkify enabled but not installed.")

There's a typo here which originates from my suggestion. Sorry! Btw, the text was just something I just quickly made up, dont know, maybe we want it to be more elaborate somehow @chrisjsewell ?

Also, maybe it could be justified to make the exception type ModuleNotFoundError ?

or ImportError?

I'm worried about exception type. 🤔

I think I'd vote for ModuleNotFoundError (a subclass of ImportError, a bit more precise) as it's equivalent to the root cause of the issue here (the exception we suppress inmarkdown_it/main.py line 16, and never need to raise unless this line runs).

hukkin · 2020-11-18T12:12:46Z

markdown_it/rules_core/linkify.py

+        while i >= 0:
+            i -= 1
+            if i < 0:
+                break


Suggested change

while i >= 0:

i -= 1

if i < 0:

break

while i >= 1:

i -= 1

Wouldn't this do the same thing?

Right, same 😲

chrisjsewell · 2020-12-13T13:42:53Z

@hukkinj1 have you got any more notes on this?

hukkin

This looks very good to me!

setup.py

chrisjsewell

thanks @tsutsu3 😄

tsutsu3 added 3 commits November 15, 2020 14:17

Add linkify

d71b1f8

Change implementation to exclude 'linkify-it-py' from dependency pack…

1ecaa09

…ages

Fix mypy error

1f15019

tsutsu3 added 2 commits November 15, 2020 22:37

Add dummy linkify tests

667fc2e

Comment out unnecessary methods

82d6709

tsutsu3 marked this pull request as ready for review November 15, 2020 14:49

hukkin requested changes Nov 15, 2020

View reviewed changes

Improve when linkify enabled but not installed

711a9c2

This was referenced Nov 16, 2020

Plugin for task lists #75

Closed

Allow for :::admonition syntax (without curly brackets) executablebooks/MyST-Parser#271

Closed

tsutsu3 commented Nov 17, 2020

View reviewed changes

markdown_it/rules_core/linkify.py Outdated Show resolved Hide resolved

tsutsu3 marked this pull request as draft November 17, 2020 15:22

hukkin reviewed Nov 17, 2020

View reviewed changes

tsutsu3 added 4 commits November 18, 2020 00:47

Add multi lines tests

e0d4b67

Fix bug caused by a misalignment of index 'i'

4a97b09

Change exception type and fix typo

3672c67

Merge branch 'master' into feature/5/linkify

577b5c6

tsutsu3 marked this pull request as ready for review November 18, 2020 11:30

hukkin reviewed Nov 18, 2020

View reviewed changes

tsutsu3 and others added 2 commits November 18, 2020 22:07

Improve while loop index

f4458ef

Merge branch 'master' into feature/5/linkify

efcac96

chrisjsewell requested a review from hukkin December 13, 2020 13:42

hukkin previously approved these changes Dec 13, 2020

View reviewed changes

chrisjsewell reviewed Dec 13, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

Update setup.py

258b35e

chrisjsewell dismissed hukkin’s stale review via 258b35e December 13, 2020 19:38

chrisjsewell approved these changes Dec 13, 2020

View reviewed changes

Merge branch 'master' into feature/5/linkify

994c0ad

chrisjsewell merged commit 4992645 into executablebooks:master Dec 13, 2020

chrisjsewell mentioned this pull request Jan 22, 2021

Decide default configuration for MyST-Parser v0.13 jupyter-book/jupyter-book#1185

Closed

	self.linkify = LinkifyIt()
	self.linkify = linkify_it.LinkifyIt() if linkify_it else None

	raise Exception("Linkify enabed but not installed.")
	raise Exception("Linkify enabled but not installed.")

✨ NEW: Add linkify #78

✨ NEW: Add linkify #78

Uh oh!

Conversation

tsutsu3 commented Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation memo

Uh oh!

codecov bot commented Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

chrisjsewell commented Nov 15, 2020

Uh oh!

tsutsu3 commented Nov 15, 2020

Uh oh!

chrisjsewell commented Nov 15, 2020

Uh oh!

hukkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hukkin Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hukkin Nov 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsutsu3 commented Nov 16, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsutsu3 Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisjsewell commented Dec 13, 2020

Uh oh!

hukkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisjsewell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsutsu3 commented Nov 15, 2020 •

edited

Loading

codecov bot commented Nov 15, 2020 •

edited

Loading

hukkin Nov 15, 2020 •

edited

Loading

hukkin Nov 15, 2020 •

edited

Loading

tsutsu3 Nov 18, 2020 •

edited

Loading