-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
add visitor for setVariable of mysql #4228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4228 +/- ##
=========================================
Coverage ? 60.76%
Complexity ? 352
=========================================
Files ? 1013
Lines ? 17036
Branches ? 3005
=========================================
Hits ? 10352
Misses ? 6031
Partials ? 653 Continue to review full report at Codecov.
|
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.
The comment Fixes #3914
will close the #3914, but the issue is not finished yet.
@terrymanu Hi, i guess you are reviewing this PR, so is it necessary for me to give it a look? |
.../java/org/apache/shardingsphere/sql/parser/sql/statement/dal/dialect/mysql/SetStatement.java
Show resolved
Hide resolved
|
||
@RequiredArgsConstructor | ||
@Getter | ||
public class VariableExprSegment implements SQLSegment { |
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.
- Missing java doc
- Can the class be final?
private final String variable; | ||
|
||
private final String expr; | ||
|
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.
Unnecessary blank lines
|
||
@RequiredArgsConstructor | ||
@Getter | ||
public class VariableExprSegment implements SQLSegment { |
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.
We prefer do not use abbreviation.
Please rename expr
to expression
|
||
private final String variable; | ||
|
||
private final String expr; |
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.
We prefer do not use abbreviation.
Please rename expr
to expression
import java.util.List; | ||
|
||
/** | ||
* set variable statement test case. |
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.
First letter of java doc should be upper case.
@Setter | ||
public final class SetVariableStatementTestCase extends SQLParserTestCase { | ||
|
||
@XmlElement(name = "variable-expr") |
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.
Please do not use abbreviation, please rename variable-expr
to variable-expression
|
||
|
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.
Please remove unnecessary blank lines.
|
||
|
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.
Please remove unnecessary blank lines.
shardingsphere-sql-parser/shardingsphere-sql-parser-test/src/test/resources/sql/dal/show.xml
Show resolved
Hide resolved
@terrymanu What should be written? |
Maybe |
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.
There are still some place need to be fix, but we have not time to waiting.
Because of the PR I wrote will conflict with this PR.
I will merge and revise it for next PR, please pay attention.
Please pay attention for #4233 |
For #3914 .
Changes proposed in this pull request: