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

Fix #1351 #1362

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Fix #1351 #1362

merged 3 commits into from
Apr 21, 2021

Conversation

wtd2
Copy link
Contributor

@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

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.

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.

src/main/java/picocli/CommandLine.java Show resolved Hide resolved
src/test/java/picocli/Issue1351.java Outdated Show resolved Hide resolved
@wtd2
Copy link
Contributor Author

wtd2 commented Apr 21, 2021

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

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...

src/main/java/picocli/CommandLine.java Show resolved Hide resolved
@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
Contributor Author

wtd2 commented Apr 21, 2021

Thanks @remkop for merging.

remkop added a commit that referenced this pull request Apr 21, 2021
@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
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
…onCandidates` optimization"

This reverts commit 05d952d.
MarkoMackic added a commit to MarkoMackic/picocli that referenced this pull request Oct 17, 2021
…onCandidates` optimization"

This reverts commit 05d952d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: usagehelp An issue or change related to the usage help message type: enhancement ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants