Skip to content

Fix #1351 #1362

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.

Sign up for GitHub

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

Jump to bottom
Merged
remkop merged 3 commits into from
Apr 21, 2021
Merged

Fix #1351 #1362

remkop merged 3 commits into from
Apr 21, 2021

Conversation

@wtd2
Copy link

@wtd2 wtd2 commented Apr 20, 2021

Hello, I'm with @sustc11810424 and @Lanninger08 to fix issue #1351.

If we've understood it right, we just need to avoid iterating through completionCandidates unnecessarily by first checking any existence of ${COMPLETION-CANDIDATES}.

A very simple solution with a test case is presented in the PR.

@codecov
Copy link

codecov bot commented Apr 20, 2021 •
edited
Loading

Codecov Report

Merging #1362 (6c23752) into master (5ee80a3) will decrease coverage by 0.00%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1362      +/-   ##
============================================
- Coverage     93.76%   93.75%   -0.01%     
  Complexity      474      474              
============================================
  Files             2        2              
  Lines          6961     6968       +7     
  Branches       1869     1872       +3     
============================================
+ Hits           6527     6533       +6     
  Misses          147      147              
- Partials        287      288       +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/picocli/CommandLine.java 93.58% <91.66%> (-0.01%) 330.00 <0.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 5ee80a3...6c23752. Read the comment docs.

remkop
remkop requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!
This looks pretty good. I could merge this as is, but can you take a look at my comments?
There are some small enhancements that would make it even better.

@wtd2
Copy link
Author

wtd2 commented Apr 21, 2021

Hi @remkop, thanks for your kind advice. We have modified the codes according to your suggestions.

remkop
remkop requested changes Apr 21, 2021
View reviewed changes
Copy link
Owner

@remkop remkop left a comment

Choose a reason for hiding this comment

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

Almost there, I found one more optimization, please check...

@remkop remkop added this to the 4.6.2 milestone Apr 21, 2021
@remkop remkop added type: enhancement ✨ theme: usagehelp An issue or change related to the usage help message labels Apr 21, 2021
@remkop remkop merged commit 21cc124 into remkop:master Apr 21, 2021
@remkop
Copy link
Owner

remkop commented Apr 21, 2021

Merged. Thank you for the contribution! 👍
I will update the release notes when I get to my PC.

@wtd2
Copy link
Author

wtd2 commented Apr 21, 2021

Thanks @remkop for merging.

remkop added a commit that referenced this pull request Apr 21, 2021
@remkop
[#1351][#1362] update RELEASE-NOTES for completionCandidates optimi…
b196201 
…zation
@remkop
Copy link
Owner

remkop commented Apr 21, 2021

I updated the release notes now.
I mentioned all of you (@wtd2, @sustc11810424 and @Lanninger08), I hope that is okay.

Thank you again for the contribution!

MarkoMackic pushed a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
@remkop @MarkoMackic
[remkop#1351][remkop#1362] update RELEASE-NOTES for `completionCandid…
05d952d 
…ates` optimization
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
@MarkoMackic
Revert "[remkop#1351][remkop#1362] update RELEASE-NOTES for `completi…
ec21508 
…onCandidates` optimization"

This reverts commit 05d952d.
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
@MarkoMackic
Revert "[remkop#1351][remkop#1362] update RELEASE-NOTES for `completi…
6a218a7 
…onCandidates` optimization"

This reverts commit 05d952d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remkop remkop remkop requested changes

Assignees

No one assigned

Labels

theme: usagehelp An issue or change related to the usage help message type: enhancement ✨

Projects

None yet

Milestone

4.6.2

Development

Successfully merging this pull request may close these issues.

2 participants

@wtd2 @remkop