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

replace CollectionUtils deprecated method #3027

Merged
merged 23 commits into from
Jul 13, 2020
Merged

replace CollectionUtils deprecated method #3027

merged 23 commits into from
Jul 13, 2020

Conversation

CalvinKirs
Copy link
Member

Tips

What is the purpose of the pull request

replace CollectionUtils deprecated method

Verify this pull request

(Please pick either of the following options)

This pull request is already covered by existing tests, such as (please describe tests).

@CalvinKirs
Copy link
Member Author

CalvinKirs commented Jun 21, 2020

BeanUtils Check dependency license fail,so,I copied it to my project

@gabrywu
Copy link
Member

gabrywu commented Jun 21, 2020

@CalvinKirs SonarCloud Code Analysis test failed

@CalvinKirs
Copy link
Member Author

@CalvinKirs SonarCloud Code Analysis test failed

commons-bean Check dependency license fail ,so,I copied bean-map to my project,I don’t know if it’s acceptable or not, I hope to get some suggestions from you

@gabrywu
Copy link
Member

gabrywu commented Jun 21, 2020

@CalvinKirs SonarCloud Code Analysis test failed

commons-bean Check dependency license fail ,so,I copied bean-map to my project,I don’t know if it’s acceptable or not, I hope to get some suggestions from you

@CalvinKirs If the beanMap class deprecated, so there is an alternative ? Only copying it might not be a good solution

@CalvinKirs
Copy link
Member Author

@CalvinKirs SonarCloud Code Analysis test failed

commons-bean Check dependency license fail ,so,I copied bean-map to my project,I don’t know if it’s acceptable or not, I hope to get some suggestions from you

@CalvinKirs If the beanMap class deprecated, so there is an alternative ? Only copying it might not be a good solution

Thank you very much for your suggestion. You are right. In fact, I also feel that it is rude to directly copy this method, I think I can achieve the same function in other ways. I will finish it later it.

@CalvinKirs
Copy link
Member Author

I have completed the change, I’m sorry to trouble you to have time to review @gabrywu

@lgcareer
Copy link
Contributor

lgcareer commented Jun 22, 2020

Hi,you should describe detail as following when update the pom dependency.
1、Which pom dependency you removed?And which jar removed?
2、 Which pom dependency you added?And which jar added?
Except for the above,You should give the github usrl of the dependency version.

@CalvinKirs
Copy link
Member Author

Hi,you should describe detail as following when update the pom dependency.
1、Which pom dependency you removed?And which jar removed?
2、 Which pom dependency you added?And which jar added?
Except for the above,You should give the github usrl of the dependency version.

Thank you very much for your review. I didn’t remove the jar. I changed it to beanMap because the original method was deprecated. I added common-beanutils, which was originally in the parent pom, but no license was added, I fixed it It, its address is https://github.com/apache/commons-beanutils

@codecov-commenter
Copy link

Codecov Report

Merging #3027 into dev will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #3027      +/-   ##
============================================
- Coverage     33.39%   33.38%   -0.02%     
+ Complexity     2373     2371       -2     
============================================
  Files           441      441              
  Lines         20518    20518              
  Branches       2505     2505              
============================================
- Hits           6853     6850       -3     
- Misses        13018    13020       +2     
- Partials        647      648       +1     
Impacted Files Coverage Δ Complexity Δ
...dolphinscheduler/common/utils/CollectionUtils.java 92.53% <ø> (ø) 30.00 <0.00> (ø)
...he/dolphinscheduler/api/service/LoggerService.java 82.60% <0.00%> (-8.70%) 9.00% <0.00%> (-1.00%)
...dolphinscheduler/remote/future/ResponseFuture.java 55.93% <0.00%> (-1.70%) 10.00% <0.00%> (-1.00%)

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 2d0fe8a...03cd407. Read the comment docs.

Copy link
Contributor

@qiaozhanwei qiaozhanwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@qiaozhanwei
Copy link
Contributor

please resolve conflicting files . Thx

Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
good job

@sonarcloud
Copy link

sonarcloud bot commented Jul 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@CalvinKirs
Copy link
Member Author

please resolve conflicting files . Thx

Thanks for your review, I have completed the changes, please check again, Thx

@qiaozhanwei qiaozhanwei merged commit 38e7494 into apache:dev Jul 13, 2020
@davidzollo davidzollo added the enhancement New feature or request label Jul 13, 2020
@davidzollo davidzollo added this to Requirement(需求) in DolphinScheduler Work Plan via automation Jul 13, 2020
@davidzollo davidzollo moved this from Requirement(需求) to Done(当前完成的任务) in DolphinScheduler Work Plan Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants