whisperfish / rust-phonenumber

Library for parsing, formatting and validating international phone numbers.
Apache License 2.0
163 stars 56 forks source link

PartialEq/Eq implementations do not satisfy the semantics of Eq #55

Open rubdos opened 1 year ago

rubdos commented 1 year ago

Discovered in #53, by trying this:

diff --git a/src/phone_number.rs b/src/phone_number.rs
index fe028ef..8f466e0 100644
--- a/src/phone_number.rs
+++ b/src/phone_number.rs
@@ -294,10 +294,12 @@ mod test {
         };

         let formatted = number.format().mode(mode).to_string();
-        parser::parse(country_hint, &formatted).with_context(|| {
+        let parsed = parser::parse(country_hint, &formatted).with_context(|| {
             format!("parsing {number} after formatting in {mode:?} mode as {formatted}")
         })?;

+        assert_eq!(number, parsed);
+
         Ok(())
     }
 }

I discovered that the PhoneNumber Eq implementation is a bit wonky in my opinion:

---- phone_number::test::round_trip_parsing::case_2::mode_4_Mode__National stdout ----
thread 'phone_number::test::round_trip_parsing::case_2::mode_4_Mode__National' panicked at 'assertion failed: `(left == right)`
  left: `PhoneNumber { code: Code { value: 32, source: Plus }, national: NationalNumber { value: 474091150, zeros: 0 }, extension: None, carrier: None }`,
 right: `PhoneNumber { code: Code { value: 32, source: Default }, national: NationalNumber { value: 474091150, zeros: 0 }, extension: None, carrier: None }`',
src/phone_number.rs:301:9

I would expect those two numbers to satisfy Eq, but since they are parsed differently, they are not.

Should we reconsider PartialEq/Eq implementations?

rubdos commented 4 months ago

Very much dupe of #28, woops.