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 quit command to cli_wallet #1050 #63

Merged
merged 3 commits into from Jul 25, 2018

Conversation

5 participants
@cogutvalera
Copy link
Member

commented Jul 18, 2018


if (method == "quit")
{
stop();

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 18, 2018

Member

Why not break?

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 18, 2018

Member

If this would work, I think it should be placed before line 98.

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 18, 2018

Author Member

Why break and not stop ? Is cli::stop wrong or bad ?
I've placed this after line 98 because cli_wallet::quit command can run its own method before stopping completely, so should I remove quit command from wallet.cpp so in help info users won't see this command description and should I place this line before 98 ?

This comment has been minimized.

Copy link
@abitmore

abitmore Jul 18, 2018

Member

I asked because some lines above there is a break, when got an EOF.

I've placed this after line 98 because cli_wallet::quit command can run its own method before stopping completely

Makes sense. Although your code would work, I still think it's better to quit based on returned value but not the hard-coded command "quit".

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 18, 2018

Author Member

@abitmore yes you're absolutely right about hard-coded "quit" ! I thought about it too and now I think how to make it correctly with the best quality ! Thank you very much for the suggestion and your time !

This comment has been minimized.

Copy link
@cogutvalera

cogutvalera Jul 19, 2018

Author Member

changes were made

@cogutvalera cogutvalera force-pushed the cogutvalera:valera_issue_1050 branch from cfcff07 to 535fc86 Jul 19, 2018

@cogutvalera cogutvalera referenced this pull request Jul 19, 2018

Closed

add exit or quit command to cli_wallet #1050

5 of 7 tasks complete
@pmconrad
Copy link

left a comment

In-band signalling is always a bad idea. How do you know that here aren't any API calls that regularly return a value that matches SIGQUIT on any OS?

Better use a dedicated exception type for signalling.

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

@pmconrad agree with you ! You're absolutely right ! I will improve this soon !

@cogutvalera cogutvalera changed the title fixed issue #1050 add quit command to cli_wallet #1050 Jul 21, 2018

@abitmore
Copy link
Member

left a comment

Looks good for me. @pmconrad @jmjatlanta your opinion?

@jmjatlanta
Copy link

left a comment

Looks good to me as well.

@pmconrad pmconrad merged commit 5469bb9 into bitshares:master Jul 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pmconrad

This comment has been minimized.

Copy link

commented Jul 25, 2018

Thanks!

@cogutvalera

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2018

Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.