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

Enhancement: Make Cli ArgumentParsers locale configurable #8619

Open
JamesBoon opened this issue Apr 17, 2024 · 6 comments
Open

Enhancement: Make Cli ArgumentParsers locale configurable #8619

JamesBoon opened this issue Apr 17, 2024 · 6 comments

Comments

@JamesBoon
Copy link

When creating a command in dropwizard, I usually add my help texts in english. This makes sense as dropwizards help texts are english too.
But argparse4j uses Locale.getDefault() and for this reason the output is in two different languages on each non-english system.

I have two different proposals:

  1. Just change the creation of the ArgumentParser here to set the english locale:
    final ArgumentParser p = ArgumentParsers.newFor(usage).locale(Locale.ENGLISH).addHelp(false).build()
    This would be a breaking change though. But maybe possible for 5.0.0?

  2. Or: Make it possible to somehow pass more options to the creation of the ArgumentParser. Maybe by enhancing JarLocation. It could return the locale in a function like getLocale() (default: Locale.getDefault()). Then a protected Function of Application<> like getJarLocation() could be used here:
    final Cli cli = new Cli(getJarLocation(), bootstrap, System.out, System.err);
    and the line from "1." would become:
    final ArgumentParser p = ArgumentParsers.newFor(usage).locale(location.getLocale()).addHelp(false).build()

What do you think? Is one of these ways possible?

@zUniQueX
Copy link
Member

Hi @JamesBoon.

I don't think setting the Locale to Locale.ENGLISH would be a good idea. If someone overwrites the default locale, the messages would change after integrating this change.

Your second approach should be possible. The more flexible way in my opinion is to provide a general customizer for the ArgumentParserBuilder. This could be integrated via a ServiceLoader to allow changing e.g. the locale at deployment time.

@JamesBoon
Copy link
Author

If you use a ServiceLoader you would of course be very flexible. However, I fear that this leads to way too much boilerplate... perhaps there could be a standard implementation that would be easily configurable. And if you really need it you could still expand it.

I don't know what would be the best approach here.
Because just passing a few variables or a variable object to the function would be significantly less flexible, but it would also be easier to implement and there would be more control over what exactly can and may be configured.

What do you think? Which way would lead to a better, more comfortable way for a developer using it?

@JamesBoon
Copy link
Author

JamesBoon commented May 24, 2024

Hi @zUniQueX,
I finally found some time to make an example implementation of what I meant. You can see it here.
It is just a rough sketch. There is still documentation and tests missing. I just wanted to know if this way might be okay and if I should open a PR for this.

These changes will just mark the JarLocation as deprecated and will not introduce any breaking changes!

@zUniQueX
Copy link
Member

zUniQueX commented Jun 3, 2024

@JamesBoon Looks good overall.

But this only enables a few predefined options for customization. When we update the argparse4j dependency and they add new options, we'd have to add those options to our objects as well.

I thought of a simpler solution, where we expose the ArgumentParserBuilder through a customizer for full control (maybe we could add a customizer for the ArgumentParser as well).

@JamesBoon
Copy link
Author

That looks really good and would solve my specific problem. I like it! 👍

Perhaps one could combine our approaches to be able to configure the output of the version too. Maybe someone would like to have the Git hash included in the version number or similar...

@JamesBoon
Copy link
Author

@zUniQueX thank you for your support on this topic!

After experimenting a bit with the implementation using the ServiceLoader, here are my thoughts:

  • The implementation is nice, lean and completely backward compatible.
  • But using it feels strange, as in, so much effort has to be put into such a small configuration. (Conigfile + META-INF/services/...-file)
  • It introduces a new way of configuring something than is otherwise usual in the project. (e.g. using functions in Application or adding something to bootstrap)

Actually only the third point (new way of configuring something) is actually something I am a bit concerned about.
What do you think?

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

No branches or pull requests

2 participants