8000 Change `max_money` to a constant. by Kixunil · Pull Request #1410 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Change max_money to a constant. #1410

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
Dec 3, 2022
Merged

Change max_money to a constant. #1410

merged 1 commit into from
Dec 3, 2022

Conversation

Kixunil
Copy link
Collaborator
@Kixunil Kixunil commented Nov 22, 2022

The value is statically known which is better expressed as a constant. Also allows usage in const context.

@Kixunil
Change max_money to a constant.
8000
e9dffb1 
The value is statically known which is better expressed as a constant.
Also allows usage in const context.
@Kixunil Kixunil added API break This PR requires a version bump for the next release trivial Obvious, easy and quick to review (few lines or doc-only...) 1.0 Issues and PRs required or helping to stabilize the API labels Nov 22, 2022
@apoelstra
Copy link
Member

Think we should restore the original one (with Network parameter and all) so we can mark it deprecated?

apoelstra
apoelstra approved these changes Nov 22, 2022
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 e9dffb1

tcharding
tcharding approved these changes Nov 22, 2022
View reviewed changes
Copy link
Member
@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK e9dffb1

ariard
ariard approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor
@ariard ariard left a comment

Choose a reason for hiding this comment

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

ACK e9dffb1

@tcharding tcharding mentioned this pull request Dec 2, 2022
Closed
@hashmap
Copy link
Contributor
hashmap commented Dec 2, 2022

why not const fn?

@Kixunil
Copy link
Collaborator Author
Kixunil commented Dec 2, 2022

@hashmap a constant more clearly indicates "there are no weird side effects".

@hashmap
Copy link
Contributor
hashmap commented Dec 2, 2022

@Kixunil same for const fn plus no API change, not sure how important it is though

sanket1729
sanket1729 approved these changes Dec 3, 2022
View reviewed changes
Copy link
Member
@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK e9dffb1

@Kixunil
Copy link
Collaborator Author
Kixunil commented Dec 3, 2022

@hashmap you don't see whether a random fn in the code is const or not. Constants are at least upper case. :) Also the API was changed already (removing the parameter) and the crate undergoes brutal changes so no point avoiding that.

@apoelstra apoelstra merged commit 29704f2 into rust-bitcoin:master Dec 3, 2022
@Kixunil Kixunil deleted the const-max-money branch December 3, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apoelstra apoelstra apoelstra approved these changes

@sanket1729 sanket1729 sanket1729 approved these changes

@tcharding tcharding tcharding approved these changes

@ariard ariard ariard approved these changes

Assignees
No one assigned
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release trivial Obvious, easy and quick to review (few lines or doc-only...)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Kixunil @apoelstra @hashmap @sanket1729 @tcharding @ariard
0