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

Allow customizing the markdown parser #2075

Merged
merged 4 commits into from
Mar 18, 2023

Conversation

jepler
Copy link
Contributor

@jepler jepler commented Mar 15, 2023

For instance, code using Markdown might wish to create a markdown parser that does not parse embedded HTML:

def parser_factory():
    parser = MarkdownIt()
    parser.options['html'] = False
    return parser

This is needed to properly render chatgpt's markdown; there are many "knobs" of MarkdownIt and users of the textual library may need to tune them to their needs.

jepler added a commit to jepler/chap that referenced this pull request Mar 16, 2023
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like it. Just a request re the API.

@@ -509,6 +514,7 @@ def __init__(
name: str | None = None,
id: str | None = None,
classes: str | None = None,
parser_factory: Callable[[], MarkdownIt] = default_parser_factory,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this None-able. When you need to construct the parser, check for None and create MarkdownIt("gfm-like").

This means we could drop the default_parser_factory function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can rewrite it this way, sure.

Sorry for prematurely requesting your re-review.

@@ -799,6 +807,7 @@ def __init__(
name: str | None = None,
id: str | None = None,
classes: str | None = None,
parser_factory: Callable[[], MarkdownIt] = default_parser_factory,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comment as above. Let's make this None able.

@jepler jepler requested a review from willmcgugan March 16, 2023 13:41
For instance, code using Markdown might wish to create a markdown
parser that does not parse embedded HTML:
```py
def parser_factory():
    parser = MarkdownIt("gfm-like")
    parser.options["html"] = False
    return parser
```
@jepler
Copy link
Contributor Author

jepler commented Mar 16, 2023

OK -- rebased and now actually ready for re-review. Thank you for your time!

Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some pedantic requests.

@@ -509,6 +508,7 @@ def __init__(
name: str | None = None,
id: str | None = None,
classes: str | None = None,
parser_factory: Optional[Callable[[], MarkdownIt]] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use | None here, just for consistency sake...

@@ -574,7 +576,7 @@ async def update(self, markdown: str) -> None:
"""
output: list[MarkdownBlock] = []
stack: list[MarkdownBlock] = []
parser = MarkdownIt("gfm-like")
parser = self._parser_factory() if self._parser_factory else MarkdownIt("gfm-like")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this an explicit is None check please.

@jepler
Copy link
Contributor Author

jepler commented Mar 16, 2023

Thanks again for the feedback. I've updated the PR once more.

@jepler jepler requested a review from willmcgugan March 16, 2023 19:06
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks!

@willmcgugan
Copy link
Collaborator

Test fail is unrelated to this change. Merging.

@willmcgugan willmcgugan merged commit d999c69 into Textualize:main Mar 18, 2023
@jepler
Copy link
Contributor Author

jepler commented Mar 18, 2023

Thank you!

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 this pull request may close these issues.

2 participants