Pull Request Test Coverage Report for Build 8316149718

Details

• 37 of 37 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 83.927%

Totals
Change from base Build 8307227520:	0.02%
Covered Lines:	19732
Relevant Lines:	23511

💛 - Coveralls

The CI failure is real -- you're introducing a dependence on alloc in the fmt method that didn't used to be there.

In principle we shouldn't need one.

Interestingly my local CI passes -- so we've found a blind spot in it! Let me investigate..

Oh, lol, the blind spot is the entire units crate. Oops. This may also explain another "how did this get past CI?" situation last week..

I forgot what our consensus was about respecting FromStr and Display invariant? Do we make an effort to maintain it?

The CI failure is real -- you're introducing a dependence on alloc in the fmt method that didn't used to be there.

In principle we shouldn't need one.

Interestingly my local CI passes -- so we've found a blind spot in it! Let me investigate..

Yeah! Realized it was an issue with allocation but I was not able to make changes yesterday. Should be fixed now.

My tests also passed locally. Not sure why

What is the current policy on running rustfmt, my editor does auto format but when I do a diff I see so many changes related to formatting

So I also need to fix it for tests, somehow they succeed locally

What is the current policy on running rustfmt, my editor does auto format but when I do a diff I see so many changes related to formatting

Please do not commit unrelated changes. Usually there should not be very many because we have a weekly bot which does all the formatting changes, but lately we have been skipping the bot because one mantainer claims undue difficulty in rebasing.

So you will need to override your editor or be careful about what changes you commit, for now.

I forgot what our consensus was about respecting FromStr and Display invariant? Do we make an effort to maintain it?

Yes, we "make an effort". But there are some exceptional cases, most notably Script, and I don't think we've worked out an explicit policy.

@448-OG, you can use just format that would internally use the nightly formatter. In your editor is vscode, you can configure project specific settings for rust-bitcoin and set the formatter as just format

WTF, this PR reversed what was previously intentionally changed for a good reason in #716

Honestly, I'm starting to get annoyed by people changing intentional design without a proper justification. In this case part of the problem is my fault for not including a test. Good learning experience for me, I'll definitely do better.

It is common to display bitcoins using trailing zeros upto 8 decimals.

This is bad justification because people can just use {:.8} if they want that but minimal formatting is impossible to specify. Therefore this was a breaking change.

@Kixunil if you want specific behavior from the amount display traits then you need tests and very specific comments. Because this module is insanely complicated and we have had multiple complaints about it and it's impossible to tell what is "deliberate" and what is not.

I note that this claims to fix a linked bug where you replied "definitely a bug", and then alongside your comment here you commented on that issue suggesting a completely undiscoverable workaround. The linked issue also definitely is a bug because it involves Amount treating formatters differently than f64s.

Further, I don't even know what specifically you're complaining about. You say you "intentionally changed for a good reason" ...some specific aspect of formatting in a PR whose description is a "significant refactor" that "makes Amount configurable"?

In general I appreciate you retroactively reviewing PRs like this, because it lets us move forward and then double-back to fix oversights rather than having half a dozen perma-stalled PRs that prevent everyone else from rebasing. But in this case I have no idea what you're talking about.

The PR literally states:

Further, this makes all representations of numbers minimal by default,
so should be documented as a possibly-breaking change.

Though it doesn't provide justification. The justification is in #709 (which is linked).

I think we should have a proptest checking roughly that format!("{:$params}", amount.display_in(denom)) == format!("{:$params}", amount.to_sat() as f64 * 10f64.pow(denom.precision())) for any params, amount and denom.

If you can write that proptest properly I'd be thrilled. I tried doing this (as a fuzztest, not as a proptest) but I got constantly gummed up by the width parameter which behaves weirdly because of the denomination. I'm not even certain how it should behave -- does the denomination's width count against the width of the output?

This should probably be promoted to its own issue. I really would like this fuzztest to exist because it would document exactly how we believe Amount should be formatted.

behaves weirdly because of the denomination

To my knowledge it behaves correctly. However the proptest must not test the case with denomination (though maybe we can hack it).

I'm not even certain how it should behave

Width means total width of the field but is precision is too big it'll be wider. With denomination we have to subtract the length of denomination from available width. (There's a potential question whether it should truncate the denomination. I don't think so.)