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.

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

Update JCommander to the latest version #782

Closed
robertpanzer opened this issue Mar 23, 2019 · 4 comments · Fixed by #841
Closed

Update JCommander to the latest version #782

robertpanzer opened this issue Mar 23, 2019 · 4 comments · Fixed by #841
Labels
help wanted

Comments

@robertpanzer
Copy link
Member

Currently we're using org.jbeust:jcommander:1.35.
The latest version is org.jbeust:jcommands:1.72.
Simply upgrading the dependency results in some test failures, so it seems like there is some work to do,
@ysb33r
Copy link
Member

ysb33r commented Jun 15, 2019

Is it time to switch to picocli instead?

@mojavelinux
Copy link
Member

I still think we should consider whether it makes sense to reuse the CLI class from Asciidoctor itself. That would avoid the code duplication. It might turn out to be a bad idea, but I think we should at least know why we don't.

@robertpanzer
Copy link
Member Author

I think there is at least the --classpath option that is only available in AsciidoctorJ and not in Asciidoctor.
Also some parameters like --loadpath require processing before any Ruby code is called because they affect how the JRuby instance is created.

Maybe we can filter out these parameters, handle them and remove them from the call, and then invoke the original Asciidoctor CLI runner.

@robertpanzer
Copy link
Member Author

I don't know picocli, or any other good alternative.
That said I am open for any other solution, it just looks like some users are complaining about the age of the version of jcommander that we currently use.
Unfortunately it looks like this does not follow semver so that we cannot simply update the version.

Fiouz added a commit to Fiouz/asciidoctorj that referenced this issue Aug 10, 2019
@Fiouz
chore(CLI): ugprade to JCommander 1.72
4325c43 
Fixed the semantic of the error message to reflect an error on the file
name instead of a missing file.

The unit test had issues:
* Test name was misleading as it was not testing the absence of input
  file argument (which would trigger usage display) but the presence of
  an empty input file name
* It was validating a bug in the parsing library fixed in JCommander
  1.61 where empty arguments were lost - cbeust/jcommander#306

Fixes asciidoctor#782
@Fiouz Fiouz mentioned this issue Aug 10, 2019
Merged
Fiouz added a commit to Fiouz/asciidoctorj that referenced this issue Aug 10, 2019
@Fiouz
doc(changelog): mention asciidoctor#782: upgrade to JCommander 1.72
ba7cbd9
Fiouz added a commit to Fiouz/asciidoctorj that referenced this issue Aug 10, 2019
@Fiouz
doc(changelog): mention asciidoctor#782: upgrade to JCommander 1.72
c39344e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore(CLI): ugprade to JCommander 1.72 Fiouz/asciidoctorj
3 participants
@mojavelinux @ysb33r @robertpanzer