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.

Sign up for GitHub

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

Add getters for docParser/HtmlRenderer/extensions #46

Closed
wants to merge 2 commits into from
Closed

Add getters for docParser/HtmlRenderer/extensions #46

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff37d75
Add getters for docParser/HtmlRenderer/extensions
dshafik Dec 31, 2014
a96eab3
Make all private elements protected instead for easier extending
dshafik Dec 31, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/CommonMarkConverter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,20 @@ public function convertToHtml($commonMark)

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

/**
* @return DocParser
*/
public function getDocParser()
{
return $this->docParser;
}

/**
* @return HtmlRenderer
*/
public function getHtmlRenderer()
{
return $this->htmlRenderer;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm against this change, simply because CommonMarkConverter is a stupid-simple convenience wrapper for people who only care about simple conversions of CommonMark.

Anyone needing additional functionality can easily init those classes themselves and do whatever they feel like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why make it more difficult? Also, makes it easier when you think about extensions and the possibility of a factory.

Why should I have to create an environment, docParser and htmlRenderer, when all I want is to change one thing in the htmlRenderer? Instead use this and ->getHtmlRenderer() and go from there.

Copy link
Member

Choose a reason for hiding this comment

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

when all I want is to change one thing in the htmlRenderer

There's nothing you can really change though - if there was, then I'd be okay with this change.

}
15 changes: 15 additions & 0 deletions src/Environment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@

class Environment
{
/**
* @var ExtensionInterface[]
*/
protected $extensions;

/**
* @var BlockParserInterface[]
*/
Expand Down Expand Up @@ -218,6 +223,8 @@ public function createInlineParserEngine()
*/
public function addExtension(ExtensionInterface $extension)
{
$this->extensions[] = $extension;

// Block parsers
foreach ($extension->getBlockParsers() as $blockParser) {
$this->addBlockParser($blockParser);
Expand All @@ -244,6 +251,14 @@ public function addExtension(ExtensionInterface $extension)
}
}

/**
* @return ExtensionInterface[]
*/
public function getExtensions()
{
return $this->extensions;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

I thought about emulating Twig's methodology of lazy-initing the extensions, which would add this functionality (among other things), but I haven't thought of a compelling use-case for it yet.

Can you think of a situation where getExtensions() would be needed? If so I'd be happy to include it :)

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 needed it to effectively unit test — to ensure that the right extension was registered by my package.

Copy link
Member

Choose a reason for hiding this comment

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

Okay that makes sense.

* @return Environment
*/
Expand Down
8 changes: 8 additions & 0 deletions src/HtmlRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,12 @@ public function renderBlocks($blocks, $inTightList = false)

return implode($this->options['blockSeparator'], $result);
}

/**
* @return Environment
*/
public function getEnvironment()
{
return $this->environment;
}
Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong, but I don't feel there's a valid use case for this. When would something need to obtain the environment from the renderer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above, needed the environment to get the extensions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just worried about adding and supporting new public methods solely for testing purposes (no strong use-case otherwise).

}