PHPORM-439 Initialize the make:document command - #3
Conversation
… and in the symfony repo
| "keywords": ["mongodb", "maker", "odm", "dev"], | ||
| "type": "symfony-bundle", | ||
| "repositories": [ | ||
| { "type": "github", "url": "https://github.com/GromNaN/symfony-maker-bundle" } |
There was a problem hiding this comment.
I created the branch 1.x on my fork to get all the required PR together. https://github.com/GromNaN/symfony-maker-bundle/tree/1.x
paulinevos
left a comment
paulinevos
left a comment
There was a problem hiding this comment.
Some comments but I don't think it should block merging for now if it all works 🤷♀️
|
|
||
| services: | ||
| mongodb: | ||
| image: "mongodb/mongodb-atlas-local:latest" |
There was a problem hiding this comment.
Do we always want to use latest or lock this to a specific tag?
There was a problem hiding this comment.
Using the latest if fine. New versions should always be backward compatible for what we do. And I don't want to have to constantly update. Unless Dependabot can do it for us.
| DocumentClassGenerator|null $documentClassGenerator = null, | ||
| ) { | ||
| if ($generator === null) { | ||
| $this->generator = new Generator($fileManager, 'App\\'); |
There was a problem hiding this comment.
Can't we like
$this->generator = $generator ?? new Generator($fileManager, 'App\\');or is this a stylistic choice?
There was a problem hiding this comment.
Good catch, I'll use CPP also.
| $documentClassName ??= $io->askQuestion($question); | ||
|
|
||
| while ($dangerous = $this->verifyDocumentName($documentClassName)) { | ||
| if ($io->confirm(sprintf('"%s" contains one or more non-ASCII characters, which are potentially problematic with some database. It is recommended to use only ASCII characters for document names. Continue anyway?', $documentClassName), false)) { |
| if ($classExists) { | ||
| $documentPath = $this->getPathOfClass($documentClassDetails->getFullName()); | ||
| $io->text('Your document already exists! So let\'s add some new fields!'); | ||
| } else { |
There was a problem hiding this comment.
Can't we put this after L155?
| ], | ||
| ]; | ||
|
|
||
| $printSection = static function (array $sectionTypes) use ($io, &$allTypes): void { |
There was a problem hiding this comment.
Feel like this one's a little hard to read. Maybe there's some way to improve it like limiting the nesting etc?
There was a problem hiding this comment.
I'll let this refactoring for a later PR.
| private function getPropertyNames(string $class): array | ||
| { | ||
| if (! class_exists($class)) { | ||
| return []; |
There was a problem hiding this comment.
Should this not be an error?
There was a problem hiding this comment.
This is for when a new document class is created.
| "description": "Symfony MakerBundle for MongoDB ODM", | ||
| "keywords": ["mongodb", "maker", "odm", "dev"], | ||
| "type": "symfony-bundle", | ||
| "repositories": [ |
There was a problem hiding this comment.
Noted that this and the "symfony/maker-bundle": "^1@dev" requirement below are required until your upstream PRs on symfony/maker-bundle are merged.
|
|
||
| final class MakeDocument extends AbstractMaker implements InputAwareMakerInterface | ||
| { | ||
| use UidTrait; |
There was a problem hiding this comment.
Do you want a todo here to note that this file is internal? Maybe an attribute to suppress the warning (not sure if it's something that comes up in CI checks or just PHPStorm).
| if ($generator === null) { | ||
| $this->generator = new Generator($fileManager, 'App\\'); | ||
| } else { | ||
| $this->generator = $generator; | ||
| } | ||
|
|
||
| if ($documentClassGenerator === null) { | ||
| $this->documentClassGenerator = new DocumentClassGenerator($this->generator, $this->mongoDBHelper); | ||
| } else { | ||
| $this->documentClassGenerator = $documentClassGenerator; | ||
| } |
There was a problem hiding this comment.
Noted that ??= isn't possible here because the argument and properties are different variables. I assume using ?? in a one-liner would be too long, so this is more readable.
| private FileManager $fileManager, | ||
| private MongoDBHelper $mongoDBHelper, | ||
| Generator|null $generator = null, | ||
| DocumentClassGenerator|null $documentClassGenerator = null, |
There was a problem hiding this comment.
Out of curiosity, is |null preferred instead of a ? prefix (just for nullable args)?
| { | ||
| // TODO: Implement configureCommand() method. | ||
| $command | ||
| ->addArgument('name', InputArgument::OPTIONAL, sprintf('Class name of the document to create or update (e.g. <fg=yellow>%s</>)', Str::asClassName(Str::getRandomTerm()))) |
There was a problem hiding this comment.
I haven't worked with Symfony Console formatting before, but is </> shorthand for disabling all formatting switches? Is it still possible to just disable individual tags (e.g. <fg> in this case, if you wanted to leave a background color active)?
3 participants
Tests require change to Symfony MakerBundle:
MakerTestCasesymfony/maker-bundle#1770doctrine/mongodb-odm-bundlean official recipe symfony/recipes#1509