-
Notifications
You must be signed in to change notification settings - Fork 418
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
Make simplified @files JCommander-compatible #573
Comments
What is the expected behaviour? Can you provide a failing unit test? |
Will do. |
I'm planning to do a picocli 3.9 release end of this week, maybe around December 29. Do you think you can provide a failing unit test before then? |
@remkop OK, will take a look later today. Thanks for the extra motivation. :-) |
Thanks for the PR! I'll take a look ASAP. |
To be honest I had some trouble understanding what the problem was here... If I apply the PR in #577 and run the tests, I see the
The reason it fails is that the parser finds an empty String as the last argument. The This seemed perfectly fine to me, and I did not understand what the problem was until I looked at the JCommander source. It appears that JCommander drops empty lines in argument files (@-files). This means it is not possible to specify empty string parameters in this format. So if the goal is to make "simplified at files" behaviour compatible with JCommander, we should drop all empty lines, not just the trailing empty lines. Is this what you have in mind? |
@remkop If you're saying that the behavior applies to all empty lines, then I agree that it makes sense to remove them as well. (For context: I did not experience this issue myself. It was reported by one of my users, who used an old JCommander at-file with a newer version of my application, now using Picocli. And the problem the user demonstrated was the trailing empty line.) |
Ok, I’ll change the behaviour of simplified at files to:
That will make the behaviour compatible with JCommander. I’m also thinking to introduce API for this:
It may make sense to keep the system property but in the documentation mention the API instead and remove mentions of the system property. Thoughts? |
@remkop Sounds good. I appreciate the new API - I never really liked setting things through system properties. |
The new parsing behaviour that ignores empty lines and comments with leading whitespace has been pushed to master. Thanks again for the PR with the test. |
The additional API to control simplified @file format has been pushed to master. |
@remkop I built the latest master a couple minutes ago and verified that the feature works for my use case. |
Great, thanks for the confirmation! |
@remkop Thanks for fixing the issue. Looking forward to 3.9.0! |
When we did #551 , we forgot about one corner case, and that is empty lines at the end. JCommander accepts files without them, our implementation does not.
The text was updated successfully, but these errors were encountered: