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

Remove read stdin loop #4097

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Conversation

ghubstan
Copy link
Contributor

@ghubstan ghubstan commented Mar 27, 2020

Replaced the Scanner input read loop with upgraded joptsimple
dependency. Cli now takes a single, non-option program argument, runs
it and exits. Also removed the "stop client" command because there is
no input loop, but shutdown() is called for orderly channel shudown
before the jvm terminates. Also changed cmd syntax from camel case
to lowercase, mimicking bitcoin-cli.

Configured logback to supress all debug & info level netty output, and
bypassed logback to print results to System.out (replaces #4091).

Replaced the Scanner input read loop with upgraded joptsimple
dependency. Cli now takes a single, non-option program argument, runs
it and exits.  Also removed the "stop client" command because there is
no input loop, but shutdown() is called for orderly channel shudown
before the jvm terminates.  Also changed cmd syntax from camel case
to lowercase, mimicking bitcoin-cli.

Configured logback to supress all debug & info level netty output, and
bypassed logback to print results to System.out.
@ghubstan ghubstan requested a review from cbeams as a code owner March 27, 2020 15:40
Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments and suggested changes. There are additional changes I'd make as well, but time is limited and this is good enough to go forward. Thanks.

cli/src/main/java/bisq/cli/app/BisqCliMain.java Outdated Show resolved Hide resolved
cli/src/main/java/bisq/cli/app/CommandParser.java Outdated Show resolved Hide resolved
ghubstan added a commit to ghubstan/bisq that referenced this pull request Mar 30, 2020
Change member name OptionParser cmdParser -> parser.
Change client port 8888 -> 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Print help to stdout, not stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
ghubstan added a commit to ghubstan/bisq that referenced this pull request Mar 30, 2020
Change member name OptionParser cmdParser -> parser.
Change server listening port to 9998, client port to 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Print help to stdout, not stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
@ghubstan ghubstan force-pushed the remove-read-stdin-loop branch from aa5b5a0 to d3e60fa Compare March 30, 2020 15:02
ghubstan added a commit to ghubstan/bisq that referenced this pull request Mar 30, 2020
Change member name OptionParser cmdParser -> parser.
Change server listening port to 9998, client port to 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Handle help command, print help to stdout, not stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
@ghubstan ghubstan force-pushed the remove-read-stdin-loop branch from cefb679 to c451769 Compare March 30, 2020 15:49
Change member name OptionParser cmdParser -> parser.
Change server listening port to 9998, client port to 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Handle help command & print that to stdout;  print help
triggered by user error to stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
@ghubstan ghubstan force-pushed the remove-read-stdin-loop branch from c451769 to aba595b Compare March 30, 2020 15:55
@cbeams
Copy link
Contributor

cbeams commented Mar 30, 2020

Please have another look at how help / usage output is handled in BisqExecutable and co. I’m referring to hooking into JOptSimple’s built-in support here.

@cbeams
Copy link
Contributor

cbeams commented Mar 30, 2020

Please have another look at how help / usage output is handled in BisqExecutable and co. I’m referring to hooking into JOptSimple’s built-in support here.

Please disregard this message. I wrote it via mobile, where I hadn't seen @ghubstan's #4097 (comment).

@cbeams
Copy link
Contributor

cbeams commented Mar 30, 2020

I would generally recommend against force pushing on PR branches, especially where feedback is being folded in. It destroys history / information, and messes with merging / rebasing if someone (like myself) is coming along with review commits as well. I don't know why you did these force pushes, perhaps there was a great reason, just mentioning.

Copy link
Contributor

@cbeams cbeams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, the changes look good. Thanks @ghubstan.

@ripcurlx ripcurlx merged commit 5faf770 into bisq-network:master Mar 30, 2020
@ghubstan
Copy link
Contributor Author

ghubstan commented Mar 30, 2020

I would generally recommend against force pushing on PR branches, especially where feedback is being folded in. It destroys history / information, and messes with merging / rebasing if someone (like myself) is coming along with review commits as well. I don't know why you did these force pushes, perhaps there was a great reason, just mentioning.

Sorry. No good enough reason for squashing too many intermediate commits, just a lot of interruptions today, preventing me from correctly implementing all of your requested changes in one push before the end of your, @cbeams' day. I thought one clean (2nd) commit message was more important. I'll be more careful and patient next time.

@ghubstan ghubstan deleted the remove-read-stdin-loop branch March 30, 2020 18:38
@cbeams
Copy link
Contributor

cbeams commented Mar 30, 2020

All good, thanks!

@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
cbeams pushed a commit to cbeams/bisq that referenced this pull request Apr 2, 2020
Change member name OptionParser cmdParser -> parser.
Change server listening port to 9998, client port to 9998.
Change constructor argument from String[] param -> args.
Print the result only, w/out exec time.
Handle help command & print that to stdout;  print help
triggered by user error to stderr.
Use explicit system SUCCESS/FAIL codes in System.exit(0 || 1).
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants