-
Notifications
You must be signed in to change notification settings - Fork 668
Support DROP PROCEDURE statement
#1324
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
Conversation
Pull Request Test Coverage Report for Build 9666189324Details
💛 - Coveralls |
src/ast/mod.rs
Outdated
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| #[cfg_attr(feature = "visitor", derive(Visit, VisitMut))] | ||
| pub struct DropFunctionDesc { | ||
| pub struct DropFuncDesc { |
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.
was it a specific reason to rename DropFunctionDesc to DropFuncDesc? if not we can probably undo the change given its a breaking change and the benefit is arguable, (and we have other similarly named objects like DropFunctionOption, parse_drop_function_desc)
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.
Ah yeah you're right. I playing around with some new names since DropProcedure uses it. I'll change it back.
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.
done in bb0dad8
src/parser/mod.rs
Outdated
| let option = match self.parse_one_of_keywords(&[Keyword::CASCADE, Keyword::RESTRICT]) { | ||
| Some(Keyword::CASCADE) => Some(ReferentialAction::Cascade), | ||
| Some(Keyword::RESTRICT) => Some(ReferentialAction::Restrict), | ||
| _ => None, |
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.
| _ => None, | |
| Some(invalid) => return parser_err!(...) | |
| None => None, |
Should this rather return an error if the action is unexpected?
seems otherwise e.g. DROP PROCEDURE testproc SET NULL will be accepted by the parser but it will silently discard the SET NULL part (we can also include a test case demonstrating the behavior)
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 think they way that parse_one_of_keywords works, it will only return CASCADE or RESTRICT here (never Some(..)).
I added a test in e782bf5 and changed the code to try and make this clearer
DROP PROCEDURE statement
Pull Request Test Coverage Report for Build 9839024282Details
💛 - Coveralls |
|
Thank you @LorrensP-2158466 and @iffyio |
|
Thanks for doing those final changes! It's very busy at the moment, sorry. |
No problems! The best software in my opinion is a good team effort. 🚀 |
Potentially closes #1282
This introduces a new statement:
DropProcedure. It has the same fields asDropFunctionbecause it also has the same syntax.The other way i could have implemented this was using the
Dropstatement and introducing a newObjectTypecalledObjectType::Procedure, but becauseDROP PROCEDURE ...is the same (just different word) asDROP FUNCTION..., I thought this was weird to do.I also added a test case in the same way as
DropFunction.Let me know what you guys think of this!