Created a doc builder, doc config and doc result classes #71
Conversation
|
If possible, let's merge this soon and tag a new version because we want to start using this in symfony.com. Thanks! |
|
because the new maybe before we merge @weaverryan could you review this, please? |
| @@ -17,23 +17,12 @@ | |||
| class Application | |||
| { | |||
| private $application; | |||
| private $buildContext; | |||
| private $buildConfig; | |||
|
|
|||
| public function __construct(string $symfonyVersion) | |||
There was a problem hiding this comment.
We don't need this argument anymore, right? Or if we do, we should pass it directly into BuildConfig to set its version (which currently has a hardcoded default of 4.4)?
| use SymfonyDocsBuilder\Generator\HtmlForPdfGenerator; | ||
| use SymfonyDocsBuilder\Generator\JsonGenerator; | ||
|
|
||
| final class DocBuilder |
There was a problem hiding this comment.
Unless I'm missing something, this class isn't used anywhere IN the bundle. It's just a public class that can be consumed by any project. If we DO decide to keep the BuildDocsCommand, I guess that command should also (in the future) use this class.
There was a problem hiding this comment.
As far as tests go, if this is the central mechanism to build the docs, then this could/should be used in the IntegrationTest :)
|
Thanks for the review and the quick merge! @nikophil you are right that we should test more ... but given that we are the only users of this project, I think it's reasonable to be agile/fast now (while we're integrating this in symfony.com) and then make things more robust when this integration is finished. 🙏 |
This parser was originally created with this target -> "integrating it in symofny.com should require the absolute minimum number of changes". That's where the PHAR and command made a lot of sense.
However, circumstances have changed completely. Now the target is "to create the best possible PHP-based RST parser and doc builder". That means less maintenance, less things to set up, less concepts to learn etc. That's why I created this PR.
The idea is that parsing RST contents would mean to run
composer require <this-package>and then do this in your own code:Comments:
BuildResultclass is a very simple value object to abstract all the build error messages.DocBuilderis completely new ... but it's based on the command previously used to build docs.BuildConfigis a rename + simplification of the previousBuildContext. I think the new name is more precise, because this object mostly/only stores config values and no state/context.BuildConfigwhich we need inCopyImagesListener.I know this looks like a lot of changes ... but I consider it just a simplification of the current code. I thank you all for the previous work which provided a solid foundation.
Lastly, I've been using this for weeks to build bundles docs in symfony.com with success, so I consider this tested enough 🤞