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

Implement document processor #210

Merged
merged 5 commits into from
Jan 13, 2016

Conversation

marijnvdwerf
Copy link
Contributor

First attempt at a document processor. I've added a basic extension which converts Twitter links to embedded tweets.

(Still WIP)

Fixes #207

@marijnvdwerf
Copy link
Contributor Author

@colinodell If you want to convert the InlineProcessors to use the DocumentProcessor infrastructure, I'd like to see some changes in loading extensions. Currently, extensions are loaded based on name. I'd like to propose to change that to order of adding. So if you add FooExtension before BarExtension, foo gets processed first.

Other extensions probably expect Strong and Italic to be processed before starting. But if you create an extension named 'awesome-paragraphs', these processors would run before the Commonmark ones.

@colinodell
Copy link
Member

@marijnvdwerf Thank you for taking the lead on this. I apologize for the delay in reviewing this, but overall this is exactly what I've been hoping for :)

I'd like to see some changes in loading extensions. Currently, extensions are loaded based on name. I'd like to propose to change that to order of adding. So if you add FooExtension before BarExtension, foo gets processed first.

That makes sense to me, and I'm strongly in favor of that change. It seems to be a simple yet effective solution to this problem. (Otherwise you'd need to have extensions declare their dependencies, determine the proper order, check for circular references, and a bunch of other crazy things. I'd rather use the simple solution first and avoid unnecessary complexity.)

I'm not sure I like having the TwitterEmbedExtension included in the root of the project - we should probably move that into the tests directly or maybe a league/commonmark-examples project in the future. But for temporary development/testing purposes that's fine, and it's a perfect example of the kind of functionality I'm hoping this feature will provide :)

@colinodell colinodell added the enhancement New functionality or behavior label Dec 21, 2015
@marijnvdwerf marijnvdwerf force-pushed the feature/ast-processor branch 2 times, most recently from 3dc096b to 320a361 Compare January 5, 2016 11:24
@marijnvdwerf
Copy link
Contributor Author

Rebased and implemented the order-based loading. Also fixed the MiscExtension being loaded last.

Some issues though:

  • StyleCI is failing on something outside the scope of this PR.
  • I have no idea where you want to go with the InlineProcessor. Do you want to add the InlineParserContext to each InlineContainer? Currently, the context disappears when the InlineParserEngine is done parsing a container's content.
  • Would it be an idea to reorganise the ExtensionInterface to have a more logical order? e.g.
    • BlockParser
    • InlineParser
    • DocumentProcessor
    • BlockRenderer
    • InlineRenderer

@colinodell
Copy link
Member

Hey Marijn,

I'm very pleased with how these changes look! Nicely done! I especially like how you've modified the MiscExtension logic to be consistent with how extension ordering works.

StyleCI is failing on something outside the scope of this PR.

Thanks for the heads up! I have fixed this in master (see #211). Feel free to rebase onto that to clear the error.

I have no idea where you want to go with the InlineProcessor.

To be completely honest, I wasn't exactly sure what I wanted either :P Let's hold off on modifying InlineProcessor for now and leave it as-is. We can always revisit this in the future.

Would it be an idea to reorganise the ExtensionInterface to have a more logical order?

Yeah, I'm fine with reorganizing the order of those methods, especially if the new order makes more sense.


The only thing missing is that one test case marked TODO - otherwise these changes look perfect!

I'd also like to move the TwitterEmbedExtension out of this repository, and possibly into a new league/commonmark-extras library. I've posted my thoughts to the League's mailing list and would be interested in hearing your thoughts as well.

@marijnvdwerf
Copy link
Contributor Author

  • Rebased
  • Removed Twitter
  • Implemented missing test
  • Reordered methods to match processing order

@GrahamCampbell
Copy link
Member

Removed Twitter

Surely you should just squash the commits so it was never added?

@marijnvdwerf marijnvdwerf force-pushed the feature/ast-processor branch from 7f3ecf3 to d86f40a Compare January 13, 2016 11:53
@marijnvdwerf
Copy link
Contributor Author

@GrahamCampbell Done.

colinodell added a commit that referenced this pull request Jan 13, 2016
@colinodell colinodell merged commit 9ebb390 into thephpleague:master Jan 13, 2016
@colinodell
Copy link
Member

Thank you so much @marijnvdwerf for implementing these enhancements! All of them will be included in the next version which I hope to release soon (if not today then sometime this week).

colinodell added a commit that referenced this pull request Jan 14, 2016
@colinodell
Copy link
Member

I've added some documentation for this feature: http://commonmark.thephpleague.com/customization/abstract-syntax-tree/#document-processor

@marijnvdwerf marijnvdwerf deleted the feature/ast-processor branch January 15, 2016 10:38
@marijnvdwerf
Copy link
Contributor Author

rel="nofollow" would've also made sense ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality or behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants