Migrate to Doctrine's fork of jdorn/sql-formatter #971
Conversation
| public function __construct(Configuration $configuration, AbstractPlatform $platform) | ||
| { | ||
| $this->configuration = $configuration; | ||
| $this->platform = $platform; | ||
| $this->sqlFormatter = new SqlFormatter(new NullHighlighter()); |
There was a problem hiding this comment.
I hesitated to have a ctor argument for this, but then that would mean do some mocking in the test, and "Don't mock what you don't own" ™️
When I see this, I think it could be cool to have a named constructor like SqlFormatter::withoutHighlight(). This is still a nice improvement over having this false in the method call, it's more explicit.
greg0ire
force-pushed
the
migrate-to-doctrine-fork
branch
from
f2db953 to
2bc2ea0
Compare
| public function __construct(Configuration $configuration, AbstractPlatform $platform) | ||
| { | ||
| $this->configuration = $configuration; | ||
| $this->platform = $platform; | ||
| $this->sqlFormatter = new SqlFormatter(new NullHighlighter()); |
There was a problem hiding this comment.
creating the sql formatter should be done on generate when $formatted=true, having it in the constructor makes is a hard dependency
There was a problem hiding this comment.
should we move the if (! class_exists(SqlFormatter::class)) { checks inside this class?
There was a problem hiding this comment.
The exception talks about a CLI option, so I don't think we should.
We like our dependencies maintained, and jdorn/sql-formatter sadly no longer is.
greg0ire
force-pushed
the
migrate-to-doctrine-fork
branch
from
2bc2ea0 to
0eff8f8
Compare
Summary
We like our dependencies maintained, and
jdorn/sql-formattersadly nolonger is. I am targeting master, because
doctrine/sql-formatterrequires php 7.2