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

Added extensible converter #105

Merged
merged 1 commit into from Jun 16, 2015
Merged

Conversation

hason
Copy link
Contributor

@hason hason commented Jun 3, 2015

Alternative to #103

@colinodell
Copy link
Member

I do like this better than #103. Before I accept it, let me run a possible alternative by you:

What if we move CommonMarkConverter's current constructor to a static method, and then use the constructor you've provided here instead? So it would basically look like this:

class CommonMarkConverter
{
    public function __construct(DocParser $docParser, HtmlRendererInterface $htmlRenderer)
    {
        $this->docParser = $docParser;
        $this->htmlRenderer = $htmlRenderer;
    }

    public function convertToHtml($commonMark)
    {
        $documentAST = $this->docParser->parse($commonMark);

        return $this->htmlRenderer->renderBlock($documentAST);
    }

    public static function create(array $config = array())
    {
        $environment = Environment::createCommonMarkEnvironment();
        $environment->mergeConfig($config);

        $docParser = new DocParser($environment);
        $htmlRenderer = new HtmlRenderer($environment);

        return new static($docParser, $htmlRenderer);
    }
}

The downside is that it'll break BC. Technically that's okay with 0.x releases, but I've always intended this class to remain consistent while allowing the internals to change drastically.

The upside is that CommonMarkConverter will be injectable like it should be.

I'm not sure how I feel about the static method though.

Any thoughts on this? (If you like this approach, should the create() method be called something better?)

/cc @philsturgeon and @GrahamCampbell - I'd greatly appreciate any guidance you might have on this.

@colinodell
Copy link
Member

I think I've decided on the direction I'd like to go (for now anyway):

  1. Create a new class called Converter or BaseConverter.
  2. Make its constructor accept a DocParser and HtmlRendererInterface
  3. Move the convertToHtml method into it
  4. Have CommonMarkConverter extend from this class
  5. Have CommonMarkConverter's constructor create the Environment, DocParser, and HtmlRenderer, passing the latter two into the parent's constructor.

This ensures backwards-compatibility and adds the functionality you're looking for in a logical manner. It's similar to your proposed change, but with the inheritance hierarchy reversed.

@hason if you're up to making that change I will gladly accept it :) Otherwise I'll work on it sometime this week.

@colinodell colinodell removed the feedback wanted We need your input! label Jun 15, 2015
@hason
Copy link
Contributor Author

hason commented Jun 16, 2015

@colinodell Accepted and updated.

colinodell added a commit that referenced this pull request Jun 16, 2015
Added extensible converter
@colinodell colinodell merged commit cce5c4f into thephpleague:master Jun 16, 2015
@colinodell
Copy link
Member

Awesome, thanks @hason!

@hason hason deleted the converter2 branch June 19, 2015 12:20
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.

None yet

2 participants