-
Notifications
You must be signed in to change notification settings - Fork 445
Add first failing test for #551 #558
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
Conversation
|
|
||
| @Test | ||
| public void testAtFileSimplified() { | ||
| System.setProperty("picocli.useSimplifiedAtFiles", "true"); |
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.
I can do this here, because of the change above.
|
Also added a test to see if the simplified files work with quote trimming. |
| File file = findFile("/argfile-simplified-quoted.txt"); | ||
| final App app = new App(); | ||
| final CommandLine cli = new CommandLine(app); | ||
| cli.setTrimQuotes(true); |
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.
@remkop Any chance this could also be set through a system property? This way, it prevents Picocli from being used in the simple way: CommandLine.call(...)
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.
Yes sure. Do you want to provide a separate pull request for that?
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.
I'll take a look in another PR.
|
Thanks for the PR! It’s my bedtime tonight, I’ll take a look tomorrow. |
|
I took a look just now. The approach looks good, but the tests fail in the Travis CI build. Can you make the tests pass? |
|
Sorry, I just re-read and saw your first comment. So, you are asking, if I have an option like this: and I give it multiple values, like The answer is that default arity for arrays and collection options is |
|
@remkop Thanks, I completely forgot about the arity. The test is fixed now. Did you have any more tests in mind? If so, please share - otherwise I'll move on to creating the PR wrt. the quote-trimming property. |
|
I’m happy to merge this PR when the tests pass. |
Codecov Report
@@ Coverage Diff @@
## master #558 +/- ##
============================================
+ Coverage 89.04% 89.13% +0.09%
- Complexity 285 286 +1
============================================
Files 4 4
Lines 3961 3967 +6
Branches 974 976 +2
============================================
+ Hits 3527 3536 +9
+ Misses 216 212 -4
- Partials 218 219 +1
Continue to review full report at Codecov.
|
|
Merged. Thanks for the PR! |
|
FYI: I created #561 as a more strategic solution for improving the convenience methods. |
The test for #551 is failing on
List-based arguments. Before I move on with writing extra coverage, we should probably agree on how to deal with this. WDYT @remkop?