-
Notifications
You must be signed in to change notification settings - Fork 605
Adding support for parsing CREATE TRIGGER and DROP TRIGGER statements #1352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 3 commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
a3fa3b5
Added tests for the TRIGGERs
LucaCappelletti94 308b1a7
Added missing keywords needed for triggers
LucaCappelletti94 764707f
Created new module for the AST-related objects for the TRIGGERs
LucaCappelletti94 637c632
Integrated TRIGGERS in AST enum
LucaCappelletti94 e760c07
Added methods to parse triggers
LucaCappelletti94 7474836
Extended testing and resolved some corner cases in triggers
LucaCappelletti94 9fa36d9
Formatted code
LucaCappelletti94 d6c3e51
Derived visitor trait and resolved clippy code smells
LucaCappelletti94 8104de9
Resolved documentation issue and extended test coverage for Triggers
LucaCappelletti94 9c72a62
Update src/parser/mod.rs
LucaCappelletti94 5a9a19e
Added support for optional "EACH" display and extended test suite
LucaCappelletti94 d5a6af1
Removed empty spaced with cargo fmt
LucaCappelletti94 e6294c1
Renamed TRIGGER-related struct and fixed a copy-paste error and a typo
LucaCappelletti94 6f7c9c3
Reformatted code<
LucaCappelletti94 429b202
Merged DropFunctionDesc and TriggerFunctionDesc into FunctionDesc
LucaCappelletti94 dbaca3a
Updated struct documentation
LucaCappelletti94 3b26bc0
Refactored code to make use of expect_keyboard where appropriate
LucaCappelletti94 552825c
Extended test suite for multiple events
LucaCappelletti94 40993e0
Added coverage for both function and procedure in triggers
LucaCappelletti94 e26ef31
Extended trigger testing which now includes also functions with multi…
LucaCappelletti94 bf9da2a
Formatted code
LucaCappelletti94 c83dd34
Added support for deferrability in TRIGGERS and extended test suite
LucaCappelletti94 4dcee7f
Added combinatorial option for Row/Statement
LucaCappelletti94 5f4de6b
Added testing for referencing tables in TRIGGERs
LucaCappelletti94 eeb2eb0
Added some negative testig
LucaCappelletti94 b23c5fd
Added support for DropTrigger
LucaCappelletti94 2555c1e
Removed optionality of TriggerObject as it must be defined
LucaCappelletti94 8cb0ac9
Formatted code
LucaCappelletti94 47c2f40
Changed attribute event to events as it refers to a vector of events
LucaCappelletti94 e81a58c
Extended the test suite
LucaCappelletti94 c3a96e0
Added support for the TRIGGER return type
LucaCappelletti94 430a163
Update tests/sqlparser_postgres.rs
LucaCappelletti94 498aada
Update src/parser/mod.rs
LucaCappelletti94 ba5d473
Finished refactoring dialect check to simplify code
LucaCappelletti94 a27cf33
Finished test semplification changing `when` to `period`
LucaCappelletti94 3887809
Formatted code
LucaCappelletti94 459c111
Removed part of the `and_then` to adequate to codebase style
LucaCappelletti94 c043b58
Formatted code
LucaCappelletti94 3fc2684
Removed more uses of and_then and then
LucaCappelletti94 09581e7
Restored and made use in triggers of original ConstraintCharacteristics
LucaCappelletti94 058cf7f
Reverted change relative to ConstraintCharacteristics
LucaCappelletti94 30199bf
Merge branch 'main' into main
LucaCappelletti94 7ab8fdb
Update tests/sqlparser_postgres.rs
LucaCappelletti94 6c5487f
Added support for the CONSTRAINT keyword in triggers
LucaCappelletti94 c0aab06
Extended test suite and added support for CONSTRAINT keyword and FROM…
LucaCappelletti94 c198e24
Formatted code
LucaCappelletti94 d8db70e
Removed itertools dependency and tried to clean up code as much as po…
LucaCappelletti94 0f81194
Reformatted code
LucaCappelletti94 169a0be
Fixed comments
LucaCappelletti94 459dae1
Replaced complete test with partial tests
LucaCappelletti94 be1a682
Removed dead code
LucaCappelletti94 4268a9a
Merge remote-tracking branch 'origin/main' into LucaCappelletti94/main
alamb 0182a50
Fix clippy
alamb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of adding a new dependency...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was either that or a much uglier Cartesian product. I agree that I would prefer to not add dependencies, but I was asked to try and clean the combinatorial for loop used to test the CREATE TRIGGER statement. I thought that with this being a
dev-dependency
of a well-known crate, it may be okay. Do let me know which other solution you would prefer to go for.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime, I will rewrite it without the use of itertools. Do let me know which variant you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iffyio might disagree, but I think the nested
for
s are clearer than the itertools version.The various arrays should be defined separately, but then I have not problem with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the itertools dependency and tried to make a version as clean as possible in commit d8db70e. Do let me know your opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have written nested for loops, but I think your version is okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I find the flattened for-loops a bit easier to read, as otherwise, it gets to a very high level of indentation. Do let me know whether you have any more notes on the PR.