whisperfish / rust-phonenumber

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

Return unparseable country code as error instead of silent failure #74

Open rubdos opened 4 months ago

rubdos commented 4 months ago

Overrides the behaviour of https://github.com/whisperfish/rust-phonenumber/pull/54

Cc @regismesquita @costaraphael, do you think this is an appropriate error, or should we propagate the unparsable id() call too?

Fixes #56

rubdos commented 4 months ago

[..] should we propagate the unparsable id() call too?

diff --git a/src/error.rs b/src/error.rs
index c7799df..0c4fa1a 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -60,7 +60,7 @@ pub enum Parse {
     /// non-geographical entity.
     #[error("invalid country code")]
     #[allow(unused)] // This is unused in the build script
-    InvalidCountryCode,
+    InvalidCountryCode(String),

     /// This indicates the string started with an international dialing prefix,
     /// but after this was stripped from the number, had less digits than any
diff --git a/src/parser/helper.rs b/src/parser/helper.rs
index 36f73a4..acceb05 100644
--- a/src/parser/helper.rs
+++ b/src/parser/helper.rs
@@ -160,7 +160,7 @@ pub fn country_code<'a>(
                 let prefix = number.prefix.as_ref().unwrap().parse()?;

                 if database.by_code(&prefix).is_none() {
-                    return Err(error::Parse::InvalidCountryCode);
+                    return Err(error::Parse::InvalidCountryCode(prefix.to_string()));
                 } else {
                     return Ok(number);
                 }
diff --git a/src/phone_number.rs b/src/phone_number.rs
index 8907324..94dc08d 100644
--- a/src/phone_number.rs
+++ b/src/phone_number.rs
@@ -237,11 +237,17 @@ impl<'a> Country<'a> {
     }

     pub fn id(&self) -> Result<Option<country::Id>, crate::error::Parse> {
-        self.0
-            .metadata(&DATABASE)
-            .map(|m| m.id().parse())
-            .transpose()
-            .map_err(|_e| crate::error::Parse::InvalidCountryCode)
+        let metadata = if let Some(metadata) = self.0.metadata(&DATABASE) {
+            metadata
+        } else {
+            return Ok(None);
+        };
+        match metadata.id().parse() {
+            Ok(id) => Ok(Some(id)),
+            Err(e) => Err(crate::error::Parse::InvalidCountryCode(
+                metadat.id().to_string(),
+            )),
+        }
     }
 }

We'd need a separate error type, because many other points in the code trigger that error without any available context.