Improve units #2370

Kixunil · 2024-01-20T22:35:40Z

Closes #2265
Closes #2266

Disclaimer: I did this in December and don't remember why I haven't pushed it. Maybe because it's somehow broken but I don't see how so please review a bit more carefully just in case.

coveralls · 2024-01-20T22:36:42Z

Pull Request Test Coverage Report for Build 7656656308

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.02%) to 84.307%

Totals
Change from base Build 7656454620:	0.02%
Covered Lines:	19313
Relevant Lines:	22908

💛 - Coveralls

Kixunil · 2024-01-20T22:40:03Z

Oh, found it's because I was also working on #2266 and interrupted that work for some reason.

tcharding · 2024-01-22T20:31:35Z

units/src/amount.rs

+        match self {
+            ParseError::Amount(error) => write_err!(f, "invalid amount"; error),
+            ParseError::Denomination(error) => write_err!(f, "invalid denomination"; error),
+        }


Any reason why this uses different style to other code we have e.g.,

use ParseError::*; match *self { Amount(error) => write_err!(f, "invalid amount"; error), Denomination(error) => write_err!(f, "invalid denomination"; error), }

In hindsight I kind of wish we used use Self::* instead of naming the type to make maintenance slightly cheaper.

Note please this is not a my style vs your style thing, I think the style we use is clearer. (I intentionally did not change error to e.)

Various reasons:

I actually wrote this a long time ago

I didn't notice we've adopted this as official style

I don't mind typing the enum name so I wasn't trying to get rid of it

I think Swift got this one better having .Foo meaning "variant Foo of self", I wish this was available in Rust somehow!

In hindsight I kind of wish we used use Self::*

I checked and this doesn't work, that is a surprise to me.

Yeah, I hit it recently in another project. Was surprised as well.

tcharding · 2024-01-22T20:35:09Z

units/src/amount.rs

-        match *self {
-            Negative | TooBig | TooPrecise | InvalidFormat | InputTooLarge
+        match self {
+            TooPrecise | InvalidFormat | InputTooLarge
            | InvalidCharacter(_) => None,
-            InvalidDenomination(ref e) => Some(e),
+            OutOfRange(error) => Some(error),


@apoelstra do you want to please weigh in with the argument you made to me a year or so ago about why you think using *self and ref is better. Save me butchering your argument.

I personally consider ref to be mostly old style except for rare borrow checker limitations. Just using &self and not sprinkling ref everywhere is less annoying. Also this is used in most codebases I've seen so I expect newbies to be more familiar with it.

IIUC the argument for ref is that it's more accurate description of what the types are. It's not wrong but I think the fact that the matching on references is obvious (which was also the reason it got implemented; yes, I was around before it was available) makes it non-issue.

Oh, and since I experienced being forced to write ref: it was horrible. The single most annoying thing in Rust at the time. Not only forgetting about it and doing pointless edits but also harder to refactor and noisy.

@apoelstra do you want to please weigh in with the argument you made to me a year or so ago about why you think using *self and ref is better. Save me butchering your argument.

I prefer my code's semantics to be explicit. It saves nearly zero time when programming for the compiler to implicitly screw around with levels of dereferencing and literally zero time when reviewing (it may actually increase review time because it eliminates visual hints as to the types of various objects).

But since the compiler refuses to give you any help in noticing when it's inserting implicit refs and &s and *s, I've given up on trying to enforce this by hand. There's also no clippy lint for it.

With that said, and since we (I) have, over the last year, nearly completely used the *self and ref style codebase wide can we agree to keep it @Kixunil ?

I'd rather not bother with it in general but since this is top commit and it makes diff smaller I changed it.

tcharding · 2024-01-22T20:41:27Z

units/src/amount.rs

+#[cfg(feature = "std")]
+impl std::error::Error for OutOfRangeError {}


@Kixunil I put literally hours into repeatedly opening every file in the codebase that contains error code to get all the error code exactly the same so that it becomes boilerplate and devs don't have to look at it. This {} is different (in style not functionality). Can we please have a uniform policy? I don't care which it is but if you want to do this can you change every instance of struct errors (or create an issue that describes that and I'll do it).

Also we can then reasonably expect newer devs to use the correct style because its uniform.

I honestly don't believe it matters and for no-item traits I personally prefer using {} because it's shorter and no less clear than whatever else we could have.

Ok, I'm going to mentally move to using {} for structs, but keep explicit match for enums. Codebase wide will be basically an all red diff that shouldn't merge conflict too much so I'll do that at some stage when I'm too tired to do anything else.

units/src/amount.rs

Previousle we copied `unsigned_abs` method from `core` because it was unstable in older MSRV. Our current MSRV allows using the method directly so this removes our old one and uses the one from standard library instead.

The `from_str_in` methods on amounts returned `ParseAmountError` which contained `InvalidDenomination` among its variants. This one was never returned because the method doesn't parse denomination. This change separates the error out. Closes rust-bitcoin#2265

This better conveys the intention that we're checking the lower bound.

The error returned when parsing amount had a `Negative` variant which was weird/unreachable when parsing `SignedAmount`. Also weirdly, parsing would return `TooBig` when the amount was negative - too low. To resolve this we merge them into one `OutOfRange` variant that nuges the consumers to make principled decisions and print error messages as amounts being more than or less than a specific value which is easier to understand for the users. Notably, the API still allows getting information about which type was parsed and which bound was crossed but in a less obvious way. This is OK since users who have a very good reason can use this information but most won't. Closes rust-bitcoin#2266

tcharding

ACK 7bf4783

apoelstra · 2024-01-25T13:40:06Z

units/src/amount.rs

@@ -1324,6 +1419,30 @@ pub mod serde {
        fn ser_btc_opt<S: Serializer>(self, s: S) -> Result<S::Ok, S::Error>;
    }

+    struct DisplayFullError(ParseAmountError);


In 7bf4783:

I guess this is a question for a followup PR, but do we want to make this a generic thing and use it in all of our serde impls?

Relatedly, could we replace our write_err! macro with a wrapper type like this that simply has different Display impls with std and non-std?

do we want to make this a generic thing and use it in all of our serde impls?

Yes, I think so. Honestly this is the kind of thing that really should've been in core.

Relatedly, could we replace our write_err! macro with a wrapper type like this that simply has different Display impls with std and non-std?

I think that would just create more code, not less because we'd still need format_args! to supply the arguments.

apoelstra

ACK 7bf4783

apoelstra · 2024-01-25T14:17:48Z

@Kixunil your improve-units branch has a merge commit on it, which is not showing up in the pr/2370/head ref that Github provides, but which does show up when I try to merge this PR. Can you remove it and force-push?

apoelstra · 2024-01-25T14:18:48Z

Oh, it does show up, I just needed to fetch.

But since it's a straggling merge commit and neither Tobin nor I ACKed it, can you remove it?

Kixunil · 2024-01-25T15:22:56Z

What the hell? I don't see it, I tried to simply force push and it looks like it did something. Is it OK now? If not, please provide the hash of the merge commit.

apoelstra · 2024-01-25T15:56:14Z

Fixed. But for your curiosity the commit ID was fb5139b

It has @tcharding as the author -- I suspect he pushed it by accident.

tcharding · 2024-01-25T21:28:39Z

I've done stupider things before :) Was definitely not intentional. I'm surprised I have write perms to arbitrary branches on other peoples forks.

tcharding · 2024-01-25T21:32:39Z

I'll re-ack since its the same commit hash now as last time.

tcharding

ACK 7bf4783

Kixunil · 2024-01-26T09:42:35Z

I'm surprised I have write perms to arbitrary branches on other peoples forks.

As a maintainer, you do by default. (And I never disallow it.)

apoelstra · 2024-01-26T13:17:45Z

I'm surprised I have write perms to arbitrary branches on other peoples forks.

It's only branches that have PRs associated with them. Which I agree feels surprising, but it's the only way that maintainers can edit PRs.

tcharding · 2024-01-26T21:02:01Z

Oh TIL, I didn't know we could edit PRs.

Kixunil added help wanted API break This PR requires a version bump for the next release 1.0 Issues and PRs required or helping to stabilize the API C-units PRs modifying the units crate labels Jan 20, 2024

Kixunil force-pushed the improve-units branch from 9a30b76 to 1146f19 Compare January 20, 2024 23:11

Kixunil removed the help wanted label Jan 20, 2024

Kixunil mentioned this pull request Jan 21, 2024

units: Enable parsing Amount from u64::MAX #2336

Merged

Kixunil marked this pull request as draft January 21, 2024 08:57

Kixunil force-pushed the improve-units branch from 1146f19 to c1558ea Compare January 22, 2024 19:52

Kixunil marked this pull request as ready for review January 22, 2024 20:03

tcharding reviewed Jan 22, 2024

View reviewed changes

Kixunil mentioned this pull request Jan 23, 2024

units: Make amount module not require "alloc" feature #2389

Closed

Kixunil added 4 commits January 24, 2024 11:25

Clean up unsigned_abs

5e6c65b

Previousle we copied `unsigned_abs` method from `core` because it was unstable in older MSRV. Our current MSRV allows using the method directly so this removes our old one and uses the one from standard library instead.

Express i64::MAX + 1 as i64::MIN.unsigned_abs()

54cbbf8

This better conveys the intention that we're checking the lower bound.

Kixunil force-pushed the improve-units branch from c1558ea to 7bf4783 Compare January 24, 2024 10:37

tcharding previously approved these changes Jan 25, 2024

View reviewed changes

apoelstra reviewed Jan 25, 2024

View reviewed changes

apoelstra previously approved these changes Jan 25, 2024

View reviewed changes

Kixunil dismissed stale reviews from apoelstra and tcharding via 7bf4783 January 25, 2024 15:21

Kixunil force-pushed the improve-units branch from fb5139b to 7bf4783 Compare January 25, 2024 15:21

tcharding approved these changes Jan 25, 2024

View reviewed changes

apoelstra merged commit e2b9555 into rust-bitcoin:master Jan 26, 2024
62 checks passed

Kixunil deleted the improve-units branch January 26, 2024 13:22

This was referenced Apr 17, 2024

Fix kani codegen CI job #2686

Merged

Fix kani test #2430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve units #2370

Improve units #2370

Kixunil commented Jan 20, 2024 •

edited

Loading

coveralls commented Jan 20, 2024 •

edited

Loading

Kixunil commented Jan 20, 2024

tcharding Jan 22, 2024 •

edited

Loading

tcharding Jan 22, 2024

Kixunil Jan 22, 2024

tcharding Jan 24, 2024

Kixunil Jan 24, 2024

tcharding Jan 22, 2024

Kixunil Jan 22, 2024 •

edited

Loading

apoelstra Jan 23, 2024

tcharding Jan 24, 2024

Kixunil Jan 24, 2024

tcharding Jan 22, 2024

tcharding Jan 22, 2024

Kixunil Jan 22, 2024

tcharding Jan 24, 2024 •

edited

Loading

tcharding left a comment

apoelstra Jan 25, 2024

Kixunil Jan 25, 2024

apoelstra left a comment

apoelstra commented Jan 25, 2024

apoelstra commented Jan 25, 2024

Kixunil commented Jan 25, 2024

apoelstra commented Jan 25, 2024

tcharding commented Jan 25, 2024

tcharding commented Jan 25, 2024

tcharding left a comment

Kixunil commented Jan 26, 2024

apoelstra commented Jan 26, 2024

tcharding commented Jan 26, 2024

		#[cfg(feature = "std")]
		impl std::error::Error for OutOfRangeError {}

Improve units #2370

Improve units #2370

Conversation

Kixunil commented Jan 20, 2024 • edited Loading

coveralls commented Jan 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 7656656308

💛 - Coveralls

Kixunil commented Jan 20, 2024

tcharding Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil Jan 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 25, 2024

apoelstra commented Jan 25, 2024

Kixunil commented Jan 25, 2024

apoelstra commented Jan 25, 2024

tcharding commented Jan 25, 2024

tcharding commented Jan 25, 2024

tcharding left a comment

Choose a reason for hiding this comment

Kixunil commented Jan 26, 2024

apoelstra commented Jan 26, 2024

tcharding commented Jan 26, 2024

Kixunil commented Jan 20, 2024 •

edited

Loading

coveralls commented Jan 20, 2024 •

edited

Loading

tcharding Jan 22, 2024 •

edited

Loading

Kixunil Jan 22, 2024 •

edited

Loading

tcharding Jan 24, 2024 •

edited

Loading