Add option allow-empty-diff for diff command

[jawira]

Q	A
Type	feature
BC Break	no
Fixed issues	#833

Summary

--allow-no-diff --allow-empty-diff is new option for DiffCommand, it allow to display a message instead of an Exception when there's no changes in mapping information.

[lcobucci]

Thanks for your contribution, I think it needs some minor adjustment and would love to hear the reason why this is important to you.

[alcaeus]

I agree with @lcobucci here: I don't think we want to remove the exception from DiffGenerator, nor do we want to add a command line switch to control behaviour. Instead, the DiffCommand should handle the exception gracefully:

• catch it,
• let the user know that there is no diff for which a migration should be generated by printing a nice message,
• exit with a non-zero exit code

The non-zero exit code is necessary to ensure tooling can know the result of a command call, e.g. whether a migration was generated, none was necessary or another error occurred. Parsing the command output is generally not a good idea in these scenarios: we don't want people to rely on the wording of a success/error message in the output. That's what exit codes are for.

One could argue that the current exception has a similar result, but an uncaught exception always feels like an unexpected error. Not being able to generate a migration because the migrations are up-to-date with the database is not such an error case. That's why the command should gracefully handle the exception, with tooling handling the resulting return code and moving on if no special handling is necessary when no migration could be generated.

[jawira]

Thanks for your feedback, I have done requested changes.

[jawira]

Thanks for your contribution, I think it needs some minor adjustment and would love to hear the reason why this is important to you.

Well, about why I need this. To avoid any error I regularly execute three commands in a row: "migrate - diff - migrate". Because typing these three commands every time is a pain in the ass, I use buildtools to do it (i.e. Phing or Make). Thing is, buildtools rely on returned value, and when Migration throw an error all execution is stopped.

You can found some more details in original issue #833, but I am here to answer any questions you may have.

[alcaeus]

Tiny wording nitpicks, other than that this LGTM.

Can you please squash commits once you're done? Thanks!

[jawira]

Hi guys, Is there anything I can do for you to accept this PR? Thanks :)

[SenseException]

For now, it has 3 approvals and is waiting to be merged or someone else might find something in another review. Sorry, but the waiting part is the hardest part of a PR. I also know that well. Please be patient and thank you for your contribution and the time you've spent into it. It is appreciated even though it isn't merged yet.

[alcaeus]

Thanks @jawira!

[goetas]

The tests for this contribution were quite weak. It was testing only the happy path, not the case when the exception is thrown.

[didacrios]

Just a suggestion @jawira could you replace in the summary the description of the command:

It says --allow-no-diff while the command is --allow-empty-diff and maybe somebody (like a friend of mine 🙄) tries this command and went nuts not understanding why this command is not working

Thanks!

[jawira]

Just a suggestion @jawira could you replace in the summary the description of the command:

It says --allow-no-diff while the command is --allow-empty-diff and maybe somebody (like a friend of mine roll_eyes) tries this command and went nuts not understanding why this command is not working

Thanks!

@didacrios Done! :)