Skip to content
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

实现Or的准备工作 #703

Merged
merged 4 commits into from
Apr 9, 2018
Merged

实现Or的准备工作 #703

merged 4 commits into from
Apr 9, 2018

Conversation

maxiaoguang64
Copy link
Collaborator

相关issue
1、修改Conditions中conditions的类型
2、修改Conditions中add方法默认像第一个and conditions添加condition
3、修改Conditions中find方法默认获取第一个and conditions分片键相同的最后一个condition
4、修改测试用例

2、修改Conditions中add方法默认像第一个and conditions添加condition
3、修改Conditions中find方法默认获取第一个and conditions分片键相同的最后一个condition
4、修改测试用例
@codecov-io
Copy link

codecov-io commented Apr 8, 2018

Codecov Report

Merging #703 into dev will increase coverage by 0.06%.
The diff coverage is 82.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##             dev     #703      +/-   ##
=========================================
+ Coverage   67.9%   67.96%   +0.06%     
=========================================
  Files        452      454       +2     
  Lines       8653     8698      +45     
  Branches    1399     1408       +9     
=========================================
+ Hits        5876     5912      +36     
- Misses      2454     2456       +2     
- Partials     323      330       +7
Impacted Files Coverage Δ
...e/parsing/parser/context/condition/Conditions.java 90.9% <100%> (+6.29%) ⬆️
...parsing/parser/context/condition/OrConditions.java 78.94% <78.94%> (ø)
...arsing/parser/context/condition/AndConditions.java 82.35% <82.35%> (ø)
.../core/parsing/parser/context/condition/Column.java 12.5% <0%> (-12.5%) ⬇️
...rce/OrchestrationShardingDataSourceFacoryBean.java 62.5% <0%> (-12.5%) ⬇️
...io/shardingjdbc/core/parsing/SQLParsingEngine.java 66.66% <0%> (ø) ⬆️
...al/state/datasource/DataSourceListenerManager.java 54.54% <0%> (+4.54%) ⬆️
...tion/internal/config/ConfigMapListenerManager.java 65% <0%> (+5%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edde31d...5d14141. Read the comment docs.

@terrymanu
Copy link
Member

terrymanu commented Apr 8, 2018

Code review suggestion:
in class Conditions, use a object to instead of List<List<Condition>> orConditions, such as List<OrConditions>, and rename Condition to AndCondition is better?

@terrymanu terrymanu merged commit 72fb0c0 into apache:dev Apr 9, 2018
@terrymanu
Copy link
Member

terrymanu commented Apr 9, 2018

Code review suggestions:

  1. Class OrConditions and AndConditions should be final.
  2. Line 63 of OrConditions, add final for the second parameter index.
  3. Line 63 of OrConditions, maybe can simplify:
public Optional<Condition> find(final Column column, final int index) {
        Optional<AndConditions> andConditions = get(index);
        return andConditions.isPresent() ? andConditions.get().find(column) : Optional.<Condition>absent();
    }
  1. Line 78 of OrConditions, add final for parameter index.
  2. Line 87 and 89 of OrConditions, javadoc is incorrect, should be: Adjust and conditions is empty or not.
  3. Line 31 of Conditions, use @NoArgsConstructor instead of @requiredargsconstructor.
  4. Line 56 of AndConditions, cloud you just return if you find the result, maybe do not need loop all remain elements.
  5. Line 72 of AndConditions, add final for parameter index.
  6. Line 81 and 83 of AndConditions , javadoc is incorrect, should be: Adjust conditions is empty or not.
  7. Line 61 of ConditionAssert, use private instead of public.
  8. Line 56 and 65 of ConditionAssert, before assert optional.get, use assertThat(isPresent) first.
  9. Please use English for your pull request title, content and commit comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants