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

Formalize extensions support; add inline HTML support #44

Merged
merged 1 commit into from Sep 22, 2015

Conversation

srawlins
Copy link
Member

@srawlins srawlins commented Sep 7, 2015

Fixes #18 and #43

I formalized support for Markdown extensions with an extensions optional parameter on markdownToHtml() and new Document().

I then marked fenced-code-blocks as an extension (as they are), which serves as the block extension example, and added support for inline HTML (part of CommonMark 0.22), which serves as the inline extension example (and fixes #18).

The support for inline HTML is not fantastic. There seems to be a big desire for this package to be performant. Until benchmarking is more mature, I used a very very cheap (presumably), very very liberal HTML pattern. This extension is called inline-html.

if (document.extensions.contains('fenced-code-blocks')) {
blockSyntaxes.add(const FencedCodeBlockSyntax());
}
blockSyntaxes.addAll(standardBlockSyntaxes);
Copy link
Member

Choose a reason for hiding this comment

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

The way "extensions" or "plug-ins" work for InlineSyntaxes is that the parser pulls them from the document, which the public API can mutate. Outside code can just add in new objects that implement InlineSyntax.

I'm not sure if that's a great API or not—I lean towards not since it exposes an awful lot about how the parser is implemented—but I think it would be good for block syntax to be consistent with it.

You could either also add a blockSyntaxes field to Document, or remove inlineSyntaxes and use extensions for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, I'm not crazy about exposing the innards. It's also unfortunate that the order of the syntaxes matters (maybe not so much with the block-level ones), because then the user can't insert their syntax in the middle.

I'll stick with how they work for InlineSyntaxes. Adding a blockSyntaxes field to Document. I can change the API back if we need to later.

@srawlins
Copy link
Member Author

Thanks for the review, @munificent ! I've switched from strings to just objects, and I think its much cleaner now. Still not perfect, but at least consistent... let me know what you think.

@srawlins srawlins changed the title Formalize extensinos support; add inline HTML support Formalize extensions support; add inline HTML support Sep 17, 2015
@kevmoo
Copy link
Member

kevmoo commented Sep 17, 2015

@srawlins would you want to rebase against master?

@srawlins
Copy link
Member Author

I think I did... recently

@srawlins
Copy link
Member Author

Or do you mean in the commit history? No I'll squash before merging. But maybe commit history is good for review for now.

];

BlockParser(this.lines, this.document) {
_pos = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Initialize this at the declaration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@munificent
Copy link
Member

Two nits, but 👍

@munificent
Copy link
Member

Oh, and don't forget to update the CHANGELOG, adjust the version in the pubspec if needed, etc.

@srawlins
Copy link
Member Author

Thanks for the review. This starts 0.9.0-dev, which will be unstable for a few more commits, methinks.

srawlins added a commit that referenced this pull request Sep 22, 2015
Formalize extensions support; add inline HTML support
@srawlins srawlins merged commit 2870595 into dart-lang:master Sep 22, 2015
@srawlins srawlins deleted the formal-extensions-situation branch September 22, 2015 23:42
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.

Allow Inline HTML
3 participants