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 tonka exec output decoding with specified charset #39

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

Conversation

peter5he1by
Copy link

In the beginning beanshooter does not decode the output of tonka exec. It just writes the original bytes to console without any side effect. So I think it could be even better if we add an option to decode the output with specified charset to string!

PS: I kept the code format carefully because there is no editorconfig file.

@qtc-de
Copy link
Owner

qtc-de commented Nov 17, 2023

Hi @peter5he1by 👋

good stuff and kudos for investigating beanshooters option layout to place everything into the correct location 👍 I know, it is not that straight forward 😅

I would like to make three more requests:

  1. Could you please retarget the PR to the develop branch? Easiest way is probably by cherry picking your commits to it. I will create a pull request template soon to mention that the dev branch should be targeted when users create a new PR. Sorry that it was not already in place.
  2. I think we should catch the UnsupportedEncodingException that could occur when creating the InputStreamReader. This gets currently implicitly caught by catching the IOException, but the error message will not be that clear. I think catching it separately and printing a nice error would be cool. Maybe something like this:
    Logger.printlnMixedYellow("The specified encoding", charset, "is not supported.");
    Logger.printlnMixedBlue("Supported charsets include:", <list of most common charsets>, "as well as supported charsets of your Java distribution");
  3. It is probably also a good idea to add some kind of validation before the command execution. It would probably a bad user experience if the command gets executed on the server, but the output is thrown away because an invalid charset was selected. Ideally such an error would be caught by some kind of validation beforehand. I think the most elegant way would be to add this validation on the argparse level in the OptionHandler. Something like the following should work:
    if( option == TonkaBeanOption.EXEC_CHARSET )
    {
        List<String> supportedCharsets = new ArrayList<String>();
        supportedCharsets.addAll(Charset.availableCharsets().keySet());
        arg.choices(supportedCharsets);
    }
    Although this may explode in the help screen, but we will see ;)

@peter5he1by
Copy link
Author

peter5he1by commented Nov 21, 2023

Validation process in TonkaBean Dispatcher

Well I got a new idea and tried. It looks good.

If the charset is unsuppotted, the command will be executed anyway but the output could be saved.

peter5he1by@29a73a4

图片

The validation process above is embedded in the TonkaBean Dispatcher class (after the command executed).

Validation process in OptionHandler via Argument.choices

yeah it will explode in the help message if we set all available charsets as choices:

if (option == TonkaBeanOption.EXEC_CHARSET)
{
    arg.choices(new ArgumentChoice()
    {
        @Override
        public boolean contains(Object o)
        {
            try
            {
                return null != o && Charset.isSupported((String) o);
            }
            catch (IllegalCharsetNameException e)
            {
                return false;
            }
        }

        @Override
        public String textualFormat()
        {
            List<String> supportedCharsets = new ArrayList<>(Charset.availableCharsets().keySet());
            return String.join(", ",supportedCharsets);
        }
    });
}

图片

I checked the documentation of argparse4j lib and find no way to set case-insensitive choices, so an ArgumentChoice subclass is needed.

And for the big number of charsets, I think we can just put it aside bcuz how many charsets are supportted depends on the JVM itself.

@qtc-de
Copy link
Owner

qtc-de commented Dec 10, 2023

Looks good to me 👍

However, the pull request needs to be retargeted or recreated, as it does not reflect your latest changes on your develop branch :/

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