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

Fixed an issue with clibuilder not honoring the type flag GROOVY-9886 #1467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

howard-3
Copy link

No description provided.

@paulk-asert
Copy link
Contributor

This change might be okay but possibly not needed. This currently works:

def cli = new CliBuilder()
cli.a(args: 1, longOpt: 'abc', type: double, 'abc')
def options = cli.parse(['--abc', '0.0'])
assert options.abc instanceof Double

CliBuilder doesn't use the type info capability offered by commons cli but instead performs its own conversion:
https://github.com/apache/groovy/blob/master/subprojects/groovy-cli-commons/src/main/groovy/groovy/cli/commons/OptionAccessor.groovy#L118

@howard-3
Copy link
Author

Yea we probably do that if we don't need to access the type field, but currently we switch on the type info given by the option class. We're looking to move to pico cli in the future but currently this is breaking quite a lot of stuff.

@paulk-asert
Copy link
Contributor

Do you have an example of where you'd use the type?

In general, having code which relies on underlying implementation details is always harder to port compared to code which uses the published API.

I don't know whether the following helps:

def cli = new groovy.cli.commons.CliBuilder()
cli.a(args: 1, longOpt: 'abc', type: double, 'abc')
def options = cli.parse(['--abc', '0.0'])
assert options.abc instanceof Double
assert options.getSavedTypeOptions()['abc']?.type.toString() == 'double'

It illustrates how to get the type in a way the leaks some details of the CliBuilder implementation but not details from the underlying CLI library and should port unchanged across to picocli.

@howard-3
Copy link
Author

I can't really show the actual code, but it looks something like this

def a = cli.parse(..)
a.getOptions().each {
  def prop = it.getLongOpt();
  def type = it.getType();
  def val = it.getValue()
  switch (type) {
    case Number: 
      parseNumber
      break;
     case Boolean:
       boolean.valueof()
    ...
     default:
       throw new UnsupportedOperationException(...)
  }
}

based on the groovy 2 docs, it does seem to support this behavior although didn't explicitly encourage it.
http://docs.groovy-lang.org/2.4.7/html/gapi/groovy/util/CliBuilder.html#options

@paulk-asert
Copy link
Contributor

From that snippet, you are perhaps doing work you don't need to as most types are already handled. But if you really do need it, I would suggest using getSavedTypeOptions() as per my previous example.

@howard-3
Copy link
Author

Ideally, if we're starting from scratch, we'll use that, but unfortunately the snippet can only run in groovy 3 and we're trying to migrate a lot of code written in groovy 2 to work with groovy 3. What's the solution that could work in both 2 and 3?

@paulk-asert
Copy link
Contributor

The examples I showed earlier work in 2.5+ but not 2.4. I don't know if that helps.

@howard-3
Copy link
Author

yea we're still on 2.4, 2.5 has it's own fair share of issues :-|

@howard-3
Copy link
Author

howard-3 commented Jan 26, 2021

Interesting, if I switch to use groovy.util.CliBuilder, should this work as well?

the following doesn't seem to work

def cli = new groovy.util.CliBuilder()
cli.a(args: 1, longOpt: 'abc', type: double, 'abc')
def options = cli.parse(['--abc', '0.0'])
assert options.abc instanceof Double
assert options.respondsTo("getSavedTypeOptions")
assert options.getSavedTypeOptions()['abc']?.type.toString() == 'double'

@paulk-asert
Copy link
Contributor

For the deprecated groovy.util.CliBuilder, you'd need to change the last line to:

assert options.getDelegate().getSavedTypeOptions()['abc']?.type.toString() == 'double'

And that class (groovy.util variant) is removed in Groovy 4 to allow for Java9+ JPMS compliance. So probably better to jump straight to other package variants.

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

Successfully merging this pull request may close these issues.

2 participants