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.

Sign up for GitHub

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

Jump to bottom

Make buy-xmr the default behaviour #273

Closed
wants to merge 5 commits into from
Closed

Make buy-xmr the default behaviour #273

wants to merge 5 commits into from

Conversation

rishflab
Copy link
Member

@rishflab rishflab commented Mar 4, 2021 •
edited

Closes #234

@rishflab rishflab changed the title Rename swap cli Make buy-xmr the default behaviour Mar 4, 2021
thomaseizinger
thomaseizinger reviewed Mar 4, 2021
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Interesting! Thanks for looking into this!

Had an idea that could reduce the duplication a bit. Let me know what you think.

swap/src/cli/command.rs Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger reviewed Mar 4, 2021
View reviewed changes
swap/Cargo.toml Outdated Show resolved Hide resolved
@rishflab rishflab marked this pull request as ready for review March 4, 2021 07:47
@rishflab rishflab force-pushed the rename-swap-cli branch 3 times, most recently from 23085ac to e843d58 Compare March 4, 2021 08:23
@rishflab
Default to buy-xmr if a subcommand is not specified
c4b14b2 
A new struct, CliExecutionParams, was created to represent the execution
path of the program and separate program execution from Cli argument
parsing.
@rishflab
Make bob's binary name meaningful
9e351f7 
Since buy_xmr is the default behaviour we can and should rename the
binary to buy_xmr.
@rishflab
Reduce code duplication in command module
b9b60f4 
Move alice connection args to struct. Move config and debug args to
struct.
@rishflab
Optimise command module layout
0535478 
The parsing is the most important thing it should come first.
@rishflab
Test parsing of raw command line arguments into program arguments
948cc99 
Changed the parse args function to support these tests
thomaseizinger
thomaseizinger previously approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Nice! Good that we got some tests in :)

I think you will like this @da-kami !

da-kami
da-kami previously approved these changes Mar 5, 2021
View reviewed changes
@da-kami da-kami self-requested a review March 5, 2021 04:41
@da-kami
Copy link
Member

da-kami commented Mar 5, 2021

Sorry to be the party pooper for putting my veto in for this change. I think the change from swap buy-xmr --receive-address to buy_xmr --receive-address does not justify the changes that make the command structure more complicated.
Additionally, I think we don't gain much by renaming the swap binary and I think the binary should actually be called swap and not buy_xmr because eventually we want to add selling. Yes, at the moment it is not there yet, but I still think the binary should not be named buy_xmr. I think this requires further discussion :)

@thomaseizinger thomaseizinger dismissed their stale review March 7, 2021 22:11

Changed my mind.

@thomaseizinger
Copy link
Contributor

@rishflab Thanks for putting this effort in! I actually agree with @da-kami in that swap works better as the binary name and makes it easier to extend once we add selling.

That being said, I think we made some great progress in this PR in regards to testing. Would you mind porting that to current master?

@thomaseizinger
Copy link
Contributor

Let's close this for now. The rename to swap already happened and with the outlook of eventually introducing sell-xmr, we should not be changing the binary name.

@rishflab It would still be good to get tests in for the commandline parsing if you get around to it.

@thomaseizinger thomaseizinger mentioned this pull request Mar 11, 2021
Closed
@thomaseizinger thomaseizinger deleted the rename-swap-cli branch September 3, 2021 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger left review comments

@da-kami da-kami Awaiting requested review from da-kami

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make buy-xmr the default behaviour when starting the CLI
3 participants
@rishflab @da-kami @thomaseizinger