Add Markdown Environment and Extensions support #7
Conversation
|
nightly build is broken, latest ( |
|
cc @jjergus, anything needed here ? |
fredemmott
left a comment
fredemmott
left a comment
There was a problem hiding this comment.
Sorry I missed this before; just one inline thing.
Other than that, it feels unnecessary, but I'm not strongly against it.
| private Renderer<T> $renderer, | ||
| ) {} | ||
|
|
||
| public static function html(bool $unsafe = false): Environment<string> { |
There was a problem hiding this comment.
In general, we consider bool parameters to be an antipattern
For this particular one, we definitely want the obvious explicit naming at the ca llers
There was a problem hiding this comment.
This would probably also be better as a function than a static method - then new ones can be added by third-party libraries and created in a separate library
There was a problem hiding this comment.
Agreed, I think this coud be split into 2 functions, html and unsafeHtml or something like that, instead of the bool flag.
| private Renderer<T> $renderer, | ||
| ) {} | ||
|
|
||
| public static function html(bool $unsafe = false): Environment<string> { |
There was a problem hiding this comment.
Agreed, I think this coud be split into 2 functions, html and unsafeHtml or something like that, instead of the bool flag.
| return new self($parser, $context, $renderer); | ||
| } | ||
|
|
||
| public function setParser(ParserContext $parser): void { |
There was a problem hiding this comment.
Do we need these getters and setters? I would definitely remove the setters at least (we don't want someone to change the context in the middle of rendering, or really at any point after it's assigned in the constructor).
|
Closing due to unresolved feedback. |
4 participants
No description provided.