Fix #404 - Implement ALTER EVENT

[niconoe-]

Hello all.

I'm working IRL with @SanaRebhi and we tried to make a proposal in order to solve this topic.

I made some manual tests and it looks like it's OK, but I want to go deeper and add unit tests also to cover (almost) all test cases about ALTER EVENT statements, which I don't have time right now.

Current unit-test suite is passing (with 2 ignored test-cases out of this range) locally.

Please feel free to tell me if this approch is a good one and I can continue to write tests or if you have another way in mind to manage it.

Thanks a lot.

Fixes: #404

[codecov]

Codecov Report

Base: 97.25% // Head: 97.25% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (5790812) compared to base (9306568).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #408   +/-   ##
=========================================
  Coverage     97.25%   97.25%           
- Complexity     2192     2194    +2     
=========================================
  Files            68       68           
  Lines          5055     5057    +2     
=========================================
+ Hits           4916     4918    +2     
  Misses          139      139           

Impacted Files	Coverage Δ
src/Components/AlterOperation.php	100.00% <100.00%> (ø)
src/Statements/AlterStatement.php	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[williamdes]

I think @iifawzi would better know if this feels right or not
But from what I see it's nice

[niconoe-]

I added some unit tests, I hope I did it well.

AFAIK, this is now ready for review.

Just to let you know, there are just 1 drawback I have found:
When you have an ALTER EVENT statement with dissociative options (like ENABLE and DISABLE at the same time), those options are not entering into conflict even if their IDs are declared as the same. As this behavior is generic to all parsing options but not only on ALTER EVENT, I prefered to ignore this and try to understand why in a next issue/PR.

[williamdes]

Would you mind opening a new pull-request with all of your commits but not the two source file changes ?
Or a new one with only the two source file changes ?

This is because I would like to merge all the testing part so we can see the difference after the implementation is made and what it was before in terms of .out files.
What I always do for such PRs is

rm tests/data/**/*.out
./tools/run_generators.sh

To have the changes after a new implementation on all out files

[niconoe-]

Would you mind opening a new pull-request with all of your commits but not the two source file changes ? Or a new one with only the two source file changes ?

This is because I would like to merge all the testing part so we can see the difference after the implementation is made and what it was before in terms of .out files. What I always do for such PRs is

rm tests/data/**/*.out
./tools/run_generators.sh

To have the changes after a new implementation on all out files

I’m not sure I get it.
If I send a PR without the source changes, the unit test won’t pass anymore as the .out files and the parser’s results will differ.
If I send a PR with only the source changes, the coverage report won’t like it.

I guess what you want is the list of .in files and the .out files but generated from a previous state (when the source files weren’t changed) so that with this current PR (#408), you’ll see only the source files changes + updates of the .out files, and you would be capable of comparing those .out files, am I right?

I’ll do this today: providing a PR with new keywords, new unit tests about ALTER EVENT statements, but without the feature implementation. Is this ok for you?

[williamdes]

I guess what you want is the list of .in files and the .out files but generated from a previous state (when the source files weren’t changed) so that with this current PR (#408), you’ll see only the source files changes + updates of the .out files, and you would be capable of comparing those .out files, am I right?

Yes exactly
The in files generated with the current code in the master branch and the context files added

And yes the goal is to see the diff in out files with the new implementation, you got it right

I’ll do this today: providing a PR with new keywords, new unit tests about ALTER EVENT statements, but without the feature implementation. Is this ok for you?

Yes

[niconoe-]

@williamdes Here is the PR: #409. Please tell me if something is wrong, I'll try to fix it ASAP.

Thanks again.

[williamdes]

@williamdes Here is the PR: #409. Please tell me if something is wrong, I'll try to fix it ASAP.

Thanks again.

I merged #409 thank you for this effort 🎉
You can rebase and re-generate the out files :)
And git push --force

[iifawzi]

I added some unit tests, I hope I did it well.

AFAIK, this is now ready for review.

Just to let you know, there are just 1 drawback I have found: When you have an ALTER EVENT statement with dissociative options (like ENABLE and DISABLE at the same time), those options are not entering into conflict even if their IDs are declared as the same. As this behavior is generic to all parsing options but not only on ALTER EVENT, I prefered to ignore this and try to understand why in a next issue/PR.

This's an issue that impacts many of the components, you're right. only the optionsArray component has the correct checks, this's definitely an area for improvement for the rest of the components

[niconoe-]

Failing test about mutation is about network issue I would say. Would it be possible to re-run it please?

[williamdes]

Merci for your patience and hard work @niconoe- !

[niconoe-]

Merci for your patience and hard work @niconoe- !

De rien, et avec plaisir ;)