Skip to content

Grammar fix#4179

Closed
yancyribbens wants to merge 1 commit intorust-bitcoin:masterfrom
yancyribbens:grammar-fix
Closed

Grammar fix#4179
yancyribbens wants to merge 1 commit intorust-bitcoin:masterfrom
yancyribbens:grammar-fix

Conversation

@yancyribbens
Copy link
Copy Markdown
Contributor

@yancyribbens yancyribbens commented Mar 3, 2025

Pulled f19ff22 from #4158. We can change the author of the commit but it's so minor I don't think it matters.

Also couldn't help but suggest some improvements to the title while I'm here:

  • Use backticks around Amount since it's a type.
  • The use of result is unnecessarily used twice when referring to the
    return.
  • The use of "doing" and "that is" makes the sentence very verbose.

@github-actions github-actions bot added the C-units PRs modifying the units crate label Mar 3, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Mar 3, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 13641800290

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 82.838%
Totals Coverage Status
Change from base Build 13639437937: 0.0%
Covered Lines: 21165
Relevant Lines: 25550
💛 - Coveralls

@yancyribbens
Reword title
6809ea9 
* Use backticks around Amount since it's a type.
* The use of result is unnecessarily used twice when referring to the
  return.
* The use of "doing" and "that is" makes the sentence very verbose.
@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Mar 3, 2025

FWIW I tried to cherry-pick zeevick10@e876075 to retain authorship but the commit is not part of the branch anymore it seems.

tcharding
tcharding previously approved these changes Mar 3, 2025
View reviewed changes
Copy link
Copy Markdown
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 dfc4ec5

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 3, 2025

Thanks, then new wording is nice.

@tcharding tcharding mentioned this pull request Mar 3, 2025
Merged
@zeevick10
Copy link
Copy Markdown
Contributor

zeevick10 commented Mar 3, 2025

FWIW I tried to cherry-pick zeevick10@e876075 to retain authorship but the commit is not part of the branch anymore it seems.

I opened a new pull request.

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Mar 3, 2025

lol we have a small army fixing the same typo: 45a29aa

dropping that type from this PR and others can fight out who gets credit for the one line commit lol.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 3, 2025

  • Use backticks around Amount since it's a type.

Oh I missed this during first review. 'amount types' is not the same as 'Amount types'. By 'amount types' I meant to refer to both Amount and SignedAmount - I did this throughout the module.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 3, 2025 •
edited
Loading

Perhaps this is good?

//! Provides a monodic type used when mathematical operations (`core::ops`) return an amount type.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 3, 2025

FWIW I rolled these changes into #4164.

@yancyribbens
Copy link
Copy Markdown
Contributor Author

yancyribbens commented Mar 4, 2025

FWIW I rolled these changes into #4164.

I think the correct way to do that would have been to cherry pick the commit from this branch to retain attribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tcharding tcharding tcharding left review comments

Assignees

No one assigned

Labels

C-units PRs modifying the units crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yancyribbens @coveralls @tcharding @zeevick10