8000 Make space optional in amount with denomination by casey · Pull Request #1521 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Make space optional in amount with denomination #1521

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
Merged
merged 1 commit into from
Jan 4, 2023
Merged

Make space optional in amount with denomination #1521

merged 1 commit into from
Jan 4, 2023

Conversation

casey
Copy link
Contributor
@casey casey commented Dec 31, 2022

I didn't add tests for parsing with no space, but wanted to get a PR up to show the approach.

Fixes #1519.

@apoelstra
Copy link
Member

concept ACK from me, but this part of the codebase has seen a huge amount of debate that I did not participate in.

Kixunil
Kixunil reviewed Jan 2, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Concept ACK but I think it'll make things like #1406 more challenging. Should be doable though.

@casey casey force-pushed the make-space-in-amount-with-denomination-optional branch from 6cee7f3 to 3e5e879 Compare January 3, 2023 10:33
@casey casey marked this pull request as ready for review January 3, 2023 10:33
@casey casey force-pushed the make-space-in-amount-with-denomination-optional branch from 3e5e879 to da6423b Compare January 3, 2023 10:38
@casey
Copy link
Contributor Author
casey commented Jan 3, 2023

I finished up the tests and the implementation for SignedAmount, so this should be ready for review.

Kixunil
Kixunil requested changes Jan 3, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

Nice! Apart from MSRV issue this looks good.

@casey casey requested a review from Kixunil January 4, 2023 02:02
Kixunil
Kixunil requested changes Jan 4, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Unfortunately, we require that each commit passes test suite. But you'll need to at least fix clippy issue anyway, so just rebase and squash when you do.

@casey casey force-pushed the make-space-in-amount-with-denomination-optional branch from 616ae4f to 16c49df Compare January 4, 2023 19:07
@casey
Copy link
Contributor Author
casey commented Jan 4, 2023

Ahh, got it. Fixed and squashed.

apoelstra
apoelstra approved these changes Jan 4, 2023
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 16c49df

Kixunil
Kixunil approved these changes Jan 4, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 16c49df

@apoelstra apoelstra merged commit dd2091e into rust-bitcoin:master Jan 4, 2023
@casey casey deleted the make-space-in-amount-with-denomination-optional branch January 4, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

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

Successfully merging this pull request may close these issues.

Amount with denomination is hard to pass as a command line argument
3 participants
@casey @apoelstra @Kixunil
0