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

using picocli as command line parser #6068

Closed
remkop opened this issue Aug 10, 2018 · 19 comments
Closed

using picocli as command line parser #6068

remkop opened this issue Aug 10, 2018 · 19 comments

Comments

@remkop
Copy link
Contributor

@remkop remkop commented Aug 10, 2018

The checkstyle stand-alone tool currently uses Commons CLI to parse the command line.

Would there be any interest in migrating the CLI to picocli? This would give a number of advantages:

  • more declarative (and less) code in com.puppycrawl.tools.checkstyle.Main
  • usage help in ANSI colors
  • opens the possibility to add a bash/zsh command line autocompletion script for Main in the distribution
  • support for @-files (argument files) for long command lines

See the picocli github project for more.

If there is interest I can supply a pull request.

@romani
Copy link
Member

@romani romani commented Aug 12, 2018

@remkop , thanks a lot for offer.

more declarative (and less) code in com.puppycrawl.tools.checkstyle.Main

do you have example of how code become different ? do you have any link to commit to show difference ?
I briefly look at web pages and kind of see, how it will looks like , but I still do not see full picture.

usage help in ANSI colors

minor benefit but fun

opens the possibility to add a bash/zsh command line autocompletion script for Main in the distribution

might be good, but in most cases we have single character name of parameter,how we/users benefit from this auto-completion ?

support for @-files (argument files) for long command lines

might be interesting, we will unlikely to use this cool feature, I do not see use cases for us.
Users might benefit, but CLI was never our compatibility focus, we treat it as tool us mostly.


@remkop , it is hard to say you "YES" for sure on update. I need to see a resulted diff/affect to be sure.
We can approve issue (most likely we will be happy to have it), and in PR we will see deeply in details. Are
you ok with such approach?

One issue that I see for now:
this library will be our new compile dependency, unfortunately we are not modular for now, and unfortunately this dependency will be required in many many embedded solutions with checkstyle.

@rnveach , please share your opinion on this issue.

@remkop
Copy link
Contributor Author

@remkop remkop commented Aug 13, 2018

@romani Thanks for your reply.

I started to write an article to give people hints on how to migrate from Commons CLI to picocli and I was hoping to use CheckStyle as an example. Article draft (work in progress) is here: https://github.com/remkop/picocli/wiki/Migrating-from-Commons-CLI-to-picocli

(If you end up sticking with Commons CLI, that's fine, I will look elsewhere for a different example, no worries.)

Thanks for the initial positive response. The article may give you some idea, but understood you want to see the complete picture. I will provide a PR so you can judge with the full details.

@romani
Copy link
Member

@romani romani commented Aug 13, 2018

Apache cli comes from commons-cli dependency, so we are about to substitute dependency - this is good.

We need to take affect in our uber jar "xxxxx-all.jar".
Substation of dependency for maven artifact, need to be reviewed.

I will provide a PR so you can judge with the full details.

If worst case, you will have good example to show users at article what to consider to stay on commons-cli. GitHub will preserve code in PR, even you eventually remove fork from us, so discussion and code will stay forever.
Thanks a lot.

remkop added a commit to remkop/checkstyle that referenced this issue Aug 13, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Aug 13, 2018
@remkop
Copy link
Contributor Author

@remkop remkop commented Aug 13, 2018

I submitted an initial version in PR #6082 for you to take a look.

@remkop
Copy link
Contributor Author

@remkop remkop commented Aug 13, 2018

FYI, the usage help looks like this on Cygwin: https://ibb.co/j7wymU (to give some idea of the ANSI colors and styles)

remkop added a commit to remkop/checkstyle that referenced this issue Sep 12, 2018
…LI (bugfixes, test changes for different errors and usage help format)
remkop added a commit to remkop/checkstyle that referenced this issue Sep 12, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 20, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 21, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 21, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 22, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 22, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 22, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 22, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 22, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 24, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 25, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 25, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Sep 25, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Oct 29, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Oct 29, 2018
remkop added a commit to remkop/checkstyle that referenced this issue Oct 29, 2018
@remkop
Copy link
Contributor Author

@remkop remkop commented Oct 29, 2018

As discussed, the following option names changed as part of this work:

  • -gxs was replaced with -g
  • -tabWidth was replaced with --tabWidth
  • -executeIgnoredModules was replaced with --executeIgnoredModules
  • -v (the short option for --version) was replaced with -V (uppercase)
romani added a commit that referenced this issue Oct 29, 2018
@rnveach rnveach added this to the 8.15 milestone Oct 29, 2018
@romani
Copy link
Member

@romani romani commented Oct 29, 2018

@remkop , thanks a lot for your huge effort to make transition to more advanced CLI support.

This commit is good example on a transition.

Auto-completion is possible after extra steps with installed jar - https://picocli.info/autocomplete.html

@romani romani closed this Oct 29, 2018
romani added a commit that referenced this issue Oct 30, 2018
romani added a commit that referenced this issue Oct 31, 2018
rnveach added a commit that referenced this issue Oct 31, 2018
@davidburstromspotify
Copy link

@davidburstromspotify davidburstromspotify commented Nov 30, 2018

Please note that the argument to display the version changed from -v to -V which breaks backwards compatibility, between 8.14 and 8.15

@remkop
Copy link
Contributor Author

@remkop remkop commented Nov 30, 2018

@davidburstromspotify Yes, correct. This was discussed and agreed upon.

Do you mean it should be mentioned in the documentation more prominently? If so, which docs do you have in mind?

@romani
Copy link
Member

@romani romani commented Nov 30, 2018

for now we do not consider our CLI as api, we might reconsider it in future, if we collect more requests from users.

so yes it is braking change for CLI users, but for project is general is just update.

@tsjensen
Copy link
Contributor

@tsjensen tsjensen commented Dec 1, 2018

we do not consider our CLI as api

Well, scripts are gonna break ...

@romani
Copy link
Member

@romani romani commented Dec 1, 2018

We are not going to make a lot of changes, and we try to keep compatibility as much as reasonable.

@davidburstromspotify
Copy link

@davidburstromspotify davidburstromspotify commented Dec 3, 2018

I noticed something broke in our scripts when fetching the latest from Homebrew, so I checked the release notes and didn't see anything under "Breaking backward compatibility" (which makes sense as it's not considered API), so I went through the "Notes" and noticed the switch to picocli. That was the only thing indicating the cause of the problem, from the direction I came.

@romani
Copy link
Member

@romani romani commented Dec 3, 2018

@davidburstromspotify , please share how you use checkstyle CLI, we need to know how it used to be more accurate with changes, or even start to clarify it as API.

What was broken on your side ?

jack870131 added a commit to jack870131/checkstyle that referenced this issue Dec 3, 2018
jack870131 added a commit to jack870131/checkstyle that referenced this issue Dec 3, 2018
@davidburstromspotify
Copy link

@davidburstromspotify davidburstromspotify commented Dec 3, 2018

In essence, we run a git hook that invokes Checkstyle on the Java files that are in the index. It first verifies that the currently installed version is an expected one (as specified through a constant). Then it runs checkstyle -c some_config some_files -p some_properties.
It was the version check that failed. The rest worked out of the box.

@romani
Copy link
Member

@romani romani commented Dec 3, 2018

Interesting ... I always thought that build system is unlikely to install latest without manual request.
Good to know.

@davidburstromspotify
Copy link

@davidburstromspotify davidburstromspotify commented Dec 3, 2018

It doesn't. We're using Gradle to execute Checkstyle properly and it is version locked, but the overhead is so high that we're using Homebrew installations to invoke Checkstyle for the hooks only. Hence the version check to verify that the correct version is installed on the local machine.

@rnveach
Copy link
Member

@rnveach rnveach commented Apr 9, 2019

-executeIgnoredModules was replaced with --executeIgnoredModules

It seems if you forget the extra - and just do the old way of -executeIgnoredModules, picocli does not turn the option on or generate any error. It thinks you are doing the -e option and want exclude "xecuteIgnoredModules". The usage display prints that specific option as [-e=<exclude>] even though it seems the = sign is actually optional.

@remkop
Copy link
Contributor Author

@remkop remkop commented Apr 9, 2019

@rnveach That is correct. Picocli parses that as a POSIX-style short option.

One idea to deal with this is to throw a picocli ParameterException if the value for the -e option is "xecuteIgnoredModules".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants