bugdown: Extract common function from `__init__.py`.

[shubhamdhama]

• This is a WIP pr, meant for modularizing our bugdown code which is mostly residing in __init__.py, as it is growing and becoming complex, so eventually, it would become difficult to maintain.
• Since this is such a large and complex module, I'm doing changes as small as possible so that it won't break things. Before extracting InlineInterestingLinkProcessor I'm extracting some common and generic functions.
• I want feedback about a better name for actions.py(it is inspired by zerver.lib.actions.py but I'm not sure this name fits here or not).
• Also, feedback about whether I'm going in the right direction is welcomed.

(The best part of doing this is I'm getting to read each line to see if any line which isn't covered tests we don't miss imports.)

[showell]

I don't really like the name actions.py. If we're gonna split stuff out here, I think we want modules that do more specific clumps of functions, so e.g. links.py could help with all the markdown stuff related to links.

[timabbott]

I like links.py as well.

[showell]

I think I'd rename arguments.py to globals.py.

[timabbott]

I suggested the name arguments.py, because the only reason we need globals in bugdown is because python-markdown doesn't support passing arguments (and thus data) into the functions for formatting directly. So that file is and will always be the virtual arguments (managed as a global) for the markdown processor.

[timabbott]

I kinda think add_a and add_embed belong in links.py, not a new elements.py, since I don't see a clear boundary between those.

Finally, I'm concerned about whether db_data and current_message will actually work correctly with how Python scoping works; it seems possible to me that e.g. the assignment in bugdown over the None will just move the pointer, and so the links.py code will always see None (or similar). I think it might be better to do from zerver.lib.bugdown import arguments and then access e.g. arguments.db_data in the code, to ensure that everything is accessing the same data structures.

As a sidenote, while you're working on this stuff, https://github.com/zulip/zulip/issues/8843 is a good thing to work on.

[shubhamdhama]

Ohh my bad... I haven't pushed the local changes here.. Yeah, changes in this PR doesn't update the arguments so I've also made the changes same you've suggested.

[shubhamdhama]

@timabbott can you review the elements.py file again after I've pushed the changes as it includes two more functions related to creating and accessing elements of html/node. If no I would again put them into links.py.

Also, tests would fail because of some mypy error which I'm not able to resolve as this is just the copy paste of the code:

(zulip-py3-venv)vagrant@vagrant-base-trusty-amd64:/srv/zulip$ ./tools/run-mypy
zerver/lib/bugdown/elements.py:139: error: Incompatible types in assignment (expression has type "None", variable has type "Element")

[shubhamdhama]

So my mentor pointed out the bug, and I wonder how it was working before as this is the simply moving of the code: here is the git diff of the last commit:

diff --git a/zerver/lib/bugdown/elements.py b/zerver/lib/bugdown/elements.py
index fcfb14bb4..c2aadbe3c 100644
--- a/zerver/lib/bugdown/elements.py
+++ b/zerver/lib/bugdown/elements.py
@@ -132,11 +132,10 @@ def walk_tree_with_family(root: Element,
                 queue.append(ElementPair(parent=currElementPair, value=child))  # type: ignore  # Lack of Deque support in typing module for Python 3.4.3
             result = processor(child)
             if result is not None:
+                grandparent = None
                 if currElementPair.parent is not None:
                     grandparent_element = cast(ElementPair, currElementPair.parent)
                     grandparent = grandparent_element.value
-                else:
-                    grandparent = None
                 family = ElementFamily(
                     grandparent=grandparent,
                     parent=currElementPair.value,

Now my question is are my above changes makes sense or should I make value Optional:

diff --git a/zerver/lib/bugdown/elements.py b/zerver/lib/bugdown/elements.py
index c2aadbe3c..ae8ecf557 100644
--- a/zerver/lib/bugdown/elements.py
+++ b/zerver/lib/bugdown/elements.py
@@ -116,7 +116,7 @@ ResultWithFamily = NamedTuple('ResultWithFamily', [
 
 ElementPair = NamedTuple('ElementPair', [
     ('parent', Optional[Element]),
-    ('value', Element)
+    ('value', Optional[Element])
 ])
 
 def walk_tree_with_family(root: Element,

[timabbott]

I merged the first few commits; gave you other feedback in person.

[shubhamdhama]

@timabbott I've removed that elements.py commit and added another function to links.py and there are few functions about which I'm unsure to move:

1. image_preview_enabled_for_realm
2. list_of_tlds
3. url_embed_preview_enabled_for_realm

Am I missing anything else from the in-person discussion? The last point you mentioned was to move quickly to inline preview tasks so it would be great if this PR get resolved soon.

[timabbott]

I think all 3 of those belong in links.py.

[zulipbot]

Heads up @shubhamdhama, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

[shubhamdhama]

I'm may revisit this pull request in the future. However, for now, I'm closing it.