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

Add support for variable interpolation in message strings. #526

Closed
bobtiernay-okta opened this issue Oct 22, 2018 · 25 comments
Closed

Add support for variable interpolation in message strings. #526

bobtiernay-okta opened this issue Oct 22, 2018 · 25 comments

Comments

@bobtiernay-okta
Copy link
Contributor

bobtiernay-okta commented Oct 22, 2018

Similar in spirit to ${DEFAULT-VALUE} and ${COMPLETION-CANDIDATES}, it would be very useful to allow a set of variables to be registered that could then be used in message strings.

For example, currently I need to do this on my command methods in a hierarchy:

@Override
@Command(description = "Tails the " + App.SERVICE + " docker logs")
public Status logs() {
    return super.logs();
}

what would be nicer is:

@Command(description = "Tails the ${PARENT-COMMAND-NAME} docker logs")
public Status logs() {
  // ...
}

This would allow inheritance chains to allow for encapsulating command methods in the base class and thus the need to override just to change the variable value statically.

It would be nice to have some predefined variables for common things like parent command name and current command name, as well as the ability to generalize to support this use case and others.

@remkop
Copy link
Owner

remkop commented Oct 22, 2018

Would #428 be sufficient?

@bobtiernay-okta
Copy link
Contributor Author

Although system properties could work for one aspect, I don't think it's a general solution since:

Ideally there would be a way to register something similar to a PropertySource in Spring in a similar fashion to IFactory, IVersionProvider or IDefaultValueProvider. SystemPropertySource could be one, CommandPropertySource could be another for example. The basic premise would be to combine all these use cases into the same underlying mechanism.

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

I see your point. On the other hand I'm not yet convinced that this needs first-class support with a public API like PropertySource.

Providing a system property lookup like ${sys:app-service:-defaultvalue} initially will cover a lot of ground, and can later be extended with things like ${env:key:-default} for environment variables. If more people express a need for custom lookups, then we can add API and support ${custom:key:-default} lookups (prefix name tbd).

@bobtiernay-okta
Copy link
Contributor Author

That's fair, was just relaying where my mind was at.

I was also wondering if binding variables from the command itself might be an option, either by an interface or an annotation.

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

I was also wondering if binding variables from the command itself might be an option, either by an interface or an annotation.

Unsure what you mean...
Something like ${option:outputFile} would render /a/b/c if the user specified --outputFile=/a/b/c on the command line?

@bobtiernay-okta
Copy link
Contributor Author

No something like:

@Command(...)
public class MyCommand {

   @Var("foo")
   String foo;

   //
   // Set foo on construction.
   //

  @Command(description = "Uses ${foo} to describe things")
  public void action() {
    // ...
  }

}

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

That can be achieved by selecting good system property names when using the ${sys:key} lookup...

@bobtiernay-okta
Copy link
Contributor Author

bobtiernay-okta commented Oct 23, 2018

I'm not sure I follow, you mean something like:

@Command(...)
public class MyCommand {

   {
       System.setProperty("foo", "bar");
   }

  @Command(description = "Uses ${foo} to describe things")
  public void action() {
    // ...
  }

}

If so, how does something like this work with inheritance where the base class is defining the method? Wouldn't it be unreconcilable with multiple commands?

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

We could have a method:

@Command(description = "Uses ${sys:foo} to describe things")
class BaseApp {
  String getFooValue() { return "basebar"; }
}
class Subclass extends BaseApp {
  @Override String getFooValue() { return "subbar"; }
}

// at app initialization
System.setProperty("foo", getBaseApp().getFooValue());

@bobtiernay-okta
Copy link
Contributor Author

I think this is less than ideal since this information must be controlled at the app level and thus cannot be encapsulated in commands. Second, it assumes no command delegation which I assume is a common pattern (it is for us). Lastly it suffers from all the general problems of singletons and global variables.

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

Would a custom lookup ${custom:key} with a PropertySource-like API solve this?

@bobtiernay-okta
Copy link
Contributor Author

Indeed it would. Something like IPropertyProvider would be great.

@remkop remkop added this to the 3.8 milestone Oct 23, 2018
@remkop
Copy link
Owner

remkop commented Oct 23, 2018

I just realized that picocli already has PropertySource-like API, in the form of a ResourceBundle.
In addition to ${sys:key} lookups, we could support ${resource:key} lookups that would get the value from the ResourceBundle of that command.

@bobtiernay-okta
Copy link
Contributor Author

Ya that might work. I assume it possible to add a bundle programmatically?

@remkop
Copy link
Owner

remkop commented Oct 23, 2018

After thinking about this some more, I realized that #480 would also benefit from a ${COMMAND-NAME} variable. Especially a ${FULL-COMMAND-NAME} variable that would render the whole command hierarchy (like git commit) would be useful.

Would that meet your requirements?

@bobtiernay-okta
Copy link
Contributor Author

I think this plus programmatic message bundles would probably be sufficient. I think having a full parent command path might be more useful than the full command path to the current command. The place this would come in handy is command methods that need to reference peers.

@remkop
Copy link
Owner

remkop commented Oct 31, 2018

I have an implementation for String interpolation but still thinking of how to integrate this. Can I bounce this off you:

Should the methods in the model return the original Strings, so CommandSpec.description() would return {"${COMMAND-NAME} does blah and ${sys:myKey} does foo"}, and it is up to the client code where these strings are used to call Interpolator.interpolate(String) to render the variables? Or should the model methods return the strings with variables already rendered?

I don't like losing information, so I would like to do the variable replacement as late as possible, but on the other hand it becomes a bit inconvenient to use...

@bobtiernay-okta
Copy link
Contributor Author

Hmm. I guess it is preferred to keep the API surface area as small as possible hence you would just add an overload: CommandSpec.description(boolean interpolate). The other option is having a setting to change this behavior globally.

@remkop
Copy link
Owner

remkop commented Nov 9, 2018

I'll probably use a flag like CommandLine.setInterpolateVariables(boolean). I am generally not a fan of "modal" functionality but I liked the alternatives less. (The alternatives I could think of were to increase the API surface by a lot or change the return value from String/String[] to some other object that would have separate methods to provide the interpolated and the non-interpolated values: a breaking change.)

The default will be to interpolate. Setting this to false is useful for testing.

@remkop
Copy link
Owner

remkop commented Dec 7, 2018

By the way, for anyone who reads this, would you prefer ${resource:key} or ${bundle:key} as the variable naming scheme to get property values from the command’s ResourceBundle?

@remkop
Copy link
Owner

remkop commented Apr 15, 2019

TODO:

  • allow variables to be escaped (with either $${variable} or \${variable})
  • add CommandLine.setInterpolateVariables(boolean)
  • rename CommandSpec.get/setInterpolateVariables to CommandSpec.interpolateVariables
  • implement variable expansion in index and arity attributes
  • docs for default values can link to Swift or Bash (StackOverflow or Bash Hackers Wiki)

@remkop
Copy link
Owner

remkop commented Apr 15, 2019

An initial implementation of this (without the above TODOs) has landed in master.
I still need to do documentation.

This will be included in the upcoming 4.0-alpha-2 release. If anyone can try this out in the next few days and provide feedback that would be great!

Update: the above TODOs have been addressed.

TODO:

  • interpolating an escaped variable should reduce leading $: replace $${VAR} with ${VAR}
  • document escaping behaviour
  • documentation in user manual

remkop added a commit that referenced this issue Apr 17, 2019
* documentation in release notes
* add CommandLine.is/setInterpolateVariables (with tests)
* rename CommandSpec.interpolateVariables
* support variables in arity and index
* annotation processor should not resolve any variables
* initial support for escaping with $$

TODO:
* interpolating an escaped variable should reduce leading $: replace $${VAR} with ${VAR}
* document escaping behaviour
* documentation in user manual
@remkop remkop closed this as completed in ef76dc1 Apr 17, 2019
@remkop
Copy link
Owner

remkop commented Apr 17, 2019

All of the above has been implemented and documented.
Please verify.

@remkop
Copy link
Owner

remkop commented Apr 18, 2019

See draft release notes for details.

@bobtiernay-okta
Copy link
Contributor Author

Very nice work on this, and much appreciated! Will be certain to give feedback as time affords.

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

No branches or pull requests

2 participants