[DoctrineMigration] add the maker command for doctrine migration #10

Simperfit · 2017-11-17T12:57:04Z

I will add the test (need to switch room) later today.

OskarStark · 2017-11-17T13:16:58Z

src/Command/MakeMigrationCommand.php

+        }
+
+        $this->io->newLine();
+        $this->io->writeln(' <bg=green;fg=white>          </>');


why not?

$this->io->success('Success!');

I'm using the same format as the other commands, but why not I need to look

iirc, this is just a way to have a little bit prettier format that $io->success(), but I don't remember 100% :)

weaverryan

Thanks for taking this on! Very excited about it :)

weaverryan · 2017-11-17T13:44:13Z

src/Command/MakeMigrationCommand.php

+    {
+        $this
+            ->setDescription('Creates a migration with the diff or an empty migration if there are no diffs')
+            ->addOption('empty', null, InputOption::VALUE_OPTIONAL, 'Generate an empty classes', 'false')


should be false, not a string, right?

weaverryan · 2017-11-17T13:46:32Z

tests/Command/MakeMigrationCommandTest.php

+class MakeMigrationCommandTest extends TestCase
+{
+
+}


We should add a test here: https://github.com/symfony/maker-bundle/blob/master/tests/Command/FunctionalTest.php#L68

And then we might not need a separate test at all :)

weaverryan · 2017-11-17T13:47:44Z

src/Command/MakeMigrationCommand.php

+        }
+
+        $this->io->newLine();
+        $this->io->writeln(' <bg=green;fg=white>          </>');


iirc, this is just a way to have a little bit prettier format that $io->success(), but I don't remember 100% :)

weaverryan · 2017-11-17T13:50:06Z

src/Command/MakeMigrationCommand.php

+        return [];
+    }
+
+    protected function execute(InputInterface $input, OutputInterface $output)


It's not really an intention for commands to override execute(). I totally see why you did it though - this generator is a little bit different :). What about this:

Add a new protected function generateCode() in AbstractCommand and put the $this->generator->generate() code there by default.

Override that method instead of execute() in this class.

Then we won't need to repeat things, like the success message at the bottom.

weaverryan · 2017-11-17T13:54:28Z

src/Command/MakeMigrationCommand.php

+    protected function configure()
+    {
+        $this
+            ->setDescription('Creates a migration with the diff or an empty migration if there are no diffs')


After some more thinking, I think we should not automatically create an empty migration if the diff is empty. Instead, we should show a message:

No changes were detected to your mapping metadata.
If you want to generate an empty migration, re-run the command with --empty

or sth like this: make:migration --allow-empty, make:migration:diff (make::diff-migration) or make:migration:empty (make:empty-migration)

That looks too much and too complicated. Simplicity is the main feature of this bundle. Thanks!

weaverryan · 2017-11-17T13:56:14Z

src/Command/MakeMigrationCommand.php

+
+        if (true === $isEmpty) {
+            $options['command'] = 'doctrine:migrations:diff';
+            $generateMigrationCommand->run(new ArgvInput($options), $output);


Is it possible to determine the new filename that was generated? I would love to repeat that in the "next steps" message.

@alcaeus is it possible ? We may have talked about that but I can't remember

Yes, we discussed it. The command doesn't return anything and I don't think it would be good for the command to keep the name of the last generated file in its state.

The only other option that comes to mind is actively parsing the output to grab the generated filename from the output.

weaverryan · 2017-11-17T13:56:35Z

src/Resources/help/MakeMigrations.txt

@@ -0,0 +1,5 @@
+The <info>%command.name%</info> command generates a new migrations:
+
+<info>php %command.full_name% doctrine:migrations:migrate</info>


Should be make:migration

weaverryan · 2017-11-17T13:56:42Z

src/Resources/help/MakeMigrations.txt

@@ -0,0 +1,5 @@
+The <info>%command.name%</info> command generates a new migrations:


migrations -> migration

weaverryan · 2017-11-17T13:56:58Z

src/Resources/help/MakeMigrations.txt

+
+<info>php %command.full_name% doctrine:migrations:migrate</info>
+
+If the argument is missing, the command will ask for the command name interactively.


not needed. But we should probably show one other example that shows the options being used.

weaverryan · 2017-11-17T13:57:18Z

src/Command/MakeMigrationCommand.php

+ */
+final class MakeMigrationCommand extends AbstractCommand
+{
+    protected static $defaultName = 'make:migrations';


make:migration

javiereguiluz · 2017-11-17T13:58:47Z

src/Command/MakeMigrationCommand.php

+    protected function configure()
+    {
+        $this
+            ->setDescription('Creates a migration with the diff or an empty migration if there are no diffs')


Do we really need all these options? Simplicity is the main feature of this bundle. Ideally commands have zero or one option. This looks too much to me ... but I don't use migrations, so please @weaverryan review the UX/DX of this command and see if we really need all these options. Thanks!

fabpot · 2017-11-18T07:49:32Z

src/Command/MakeMigrationCommand.php

+        $this
+            ->setDescription('Creates a migration with the diff or an empty migration if there are no diffs')
+            ->addOption('empty', null, InputOption::VALUE_OPTIONAL, 'Generate an empty classes', 'false')
+            ->addOption('db', null, InputOption::VALUE_REQUIRED, 'The database connection to use for this command.')


Dots at the end of each description (same below) should be removed.

fabpot · 2017-11-18T07:50:28Z

src/Command/MakeMigrationCommand.php

+    protected function writeNextStepsMessage(array $params, ConsoleStyle $io)
+    {
+        $io->text([
+            'Next: We just generated the migration',


The We just generated the migration should probably be removed with something along the lines of "Edit FILENAME to customize the migration."

I can't have the filename since it just in the output, or is there something that i'm missing ?

fabpot · 2017-11-18T07:50:39Z

src/Command/MakeMigrationCommand.php

+    {
+        $io->text([
+            'Next: We just generated the migration',
+            'Find the documentation at <fg=yellow>http://symfony.com/doc/current/bundles/DoctrineMigrationsBundle/index.html</>'


Use https instead of http

OskarStark · 2017-11-18T10:39:20Z

src/Command/MakeMigrationCommand.php

+            $diffMigrationCommand = $this->getApplication()->find('doctrine:migrations:diff');
+            $generateMigrationCommand = $this->getApplication()->find('doctrine:migrations:migrate');
+        } catch (CommandNotFoundException $e) {
+            throw new RuntimeCommandException(sprintf("Missing package%s: to use the migrations, run: \n\ncomposer require %s", 'migrations', 'migrations'));


- Missing package%s: to use... + Missing package %s: to use...

OskarStark · 2017-11-18T10:40:06Z

src/Command/MakeMigrationCommand.php

+            throw new RuntimeCommandException(sprintf("Missing package%s: to use the migrations, run: \n\ncomposer require %s", 'migrations', 'migrations'));
+        }
+
+        if (true === $isEmpty) {


is this needed?

- if (true === $isEmpty) { + if ($isEmpty) {

I don't want it to be another type and equal to true.

Simperfit · 2017-11-20T06:29:26Z

I think there is something wrong with the code, since I don't have any arguments and it expect me to have one

weaverryan · 2017-11-24T01:10:14Z

If/when this is merged, the official docs will need to be updated.

OskarStark · 2017-11-24T06:51:11Z

src/Command/MakeMigrationCommand.php

+        }
+    }
+
+}


EOF missing

OskarStark · 2017-11-24T06:51:24Z

tests/Command/FunctionalTest.php

+
+        $this->assertContains('Success', $tester->getDisplay());
+        $this->assertContains('Version', $tester->getDisplay());
+


please remove this blank line

OskarStark · 2017-11-24T06:52:20Z

We should activate fabot.io for this repository. Could you do this @weaverryan ?

fabpot · 2017-11-24T13:24:02Z

@OskarStark fabbot activated for this repo.

weaverryan · 2017-11-29T15:36:04Z

@Simperfit Can you rebase (sorry, I changed everything!) and look at the comments from @OskarStark?

weaverryan · 2017-12-14T01:43:16Z

Replacing this with #97 - original commit included!

This PR was squashed before being merged into the 1.0-dev branch (closes #97). Discussion ---------- New make:migration command This replaces #10. There are 2 cases: (A) When there ARE schema changes: <img width="1053" alt="screen shot 2017-12-29 at 11 07 47 pm" src="https://user-images.githubusercontent.com/121003/34451290-9b0c9bb0-ecef-11e7-9a1d-41ba78c84638.png"> (B) When there are NOT schema changes: <img width="1269" alt="screen shot 2017-12-29 at 11 07 14 pm" src="https://user-images.githubusercontent.com/121003/34451291-a617113e-ecef-11e7-979f-c30f2cc3e91e.png"> This includes a BC break on `MakerInterface`, but it will only affect people who have created custom makers (and they'll get a clear error). Commits ------- f2e25e9 tweaking message 95af9fa Adding make:migration, which includes reworking MakerInterface

OskarStark reviewed Nov 17, 2017

View reviewed changes

Simperfit force-pushed the feature/add-doctrine-migration-command branch from 2c6e82e to 4480565 Compare November 17, 2017 13:25

weaverryan requested changes Nov 17, 2017

View reviewed changes

javiereguiluz reviewed Nov 17, 2017

View reviewed changes

weaverryan mentioned this pull request Nov 17, 2017

Add a maker for Doctrine migrations #1

Closed

fabpot requested changes Nov 18, 2017

View reviewed changes

OskarStark reviewed Nov 18, 2017

View reviewed changes

[DoctrineMigration] add the maker command for doctrine migration

fcb9877

Simperfit force-pushed the feature/add-doctrine-migration-command branch from 4480565 to fcb9877 Compare November 20, 2017 06:28

Simperfit changed the title ~~[DoctrineMigration - WIP] add the maker command for doctrine migration~~ [DoctrineMigration] add the maker command for doctrine migration Nov 20, 2017

weaverryan mentioned this pull request Nov 22, 2017

Doctrine update for Flex symfony/symfony-docs#8729

Merged

1 task

OskarStark reviewed Nov 24, 2017

View reviewed changes

src/Command/MakeMigrationCommand.php

}

}

}

Copy link

Collaborator

OskarStark Nov 24, 2017

Choose a reason for hiding this comment

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

EOF missing

OskarStark reviewed Nov 24, 2017

View reviewed changes

weaverryan mentioned this pull request Dec 14, 2017

New make:migration command #97

Merged

weaverryan closed this Dec 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineMigration] add the maker command for doctrine migration #10

[DoctrineMigration] add the maker command for doctrine migration #10

Simperfit commented Nov 17, 2017

OskarStark Nov 17, 2017

Simperfit Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan left a comment

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

OskarStark Nov 18, 2017

javiereguiluz Nov 18, 2017

weaverryan Nov 17, 2017

Simperfit Nov 20, 2017

alcaeus Nov 20, 2017 •

edited

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

weaverryan Nov 17, 2017

javiereguiluz Nov 17, 2017

fabpot Nov 18, 2017

fabpot Nov 18, 2017

Simperfit Nov 20, 2017

fabpot Nov 18, 2017

OskarStark Nov 18, 2017

OskarStark Nov 18, 2017

Simperfit Nov 20, 2017

Simperfit commented Nov 20, 2017

weaverryan commented Nov 24, 2017

OskarStark Nov 24, 2017

OskarStark Nov 24, 2017

OskarStark commented Nov 24, 2017

fabpot commented Nov 24, 2017

weaverryan commented Nov 29, 2017

weaverryan commented Dec 14, 2017

		@@ -0,0 +1,5 @@
		The <info>%command.name%</info> command generates a new migrations:

		<info>php %command.full_name% doctrine:migrations:migrate</info>


		<info>php %command.full_name% doctrine:migrations:migrate</info>

		If the argument is missing, the command will ask for the command name interactively.

+                      }
+                  }
+              }


		$this->assertContains('Success', $tester->getDisplay());
		$this->assertContains('Version', $tester->getDisplay());

[DoctrineMigration] add the maker command for doctrine migration #10

[DoctrineMigration] add the maker command for doctrine migration #10

Conversation

Simperfit commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alcaeus Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Simperfit commented Nov 20, 2017

weaverryan commented Nov 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark commented Nov 24, 2017

fabpot commented Nov 24, 2017

weaverryan commented Nov 29, 2017

weaverryan commented Dec 14, 2017

alcaeus Nov 20, 2017 •

edited