From 9d972d2a963518df034dd519a8b619872be25264 Mon Sep 17 00:00:00 2001
From: Deepak Cherian <deepak@earthmover.io>
Date: Thu, 15 Aug 2024 14:08:03 -0600
Subject: [PATCH] Review comments

---
 src/dataset.rs | 10 ++++++++--
 src/storage.rs | 11 +++++------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/dataset.rs b/src/dataset.rs
index 10bb0ea7..ab64a1fa 100644
--- a/src/dataset.rs
+++ b/src/dataset.rs
@@ -622,7 +622,10 @@ mod tests {
             Arc::new(ObjectStorage::new_in_memory_store()),
             Arc::new(ObjectStorage::new_local_store(Path::new(&temp_dir_name)).unwrap()),
             // Arc::new(ObjectStorage::new_s3_store_from_env("foo".to_string()).unwrap()),
-            Arc::new(ObjectStorage::new_s3_store_with_config("foo".to_string()).unwrap()),
+            Arc::new(
+                ObjectStorage::new_s3_store_with_config("testbucket".to_string())
+                    .unwrap(),
+            ),
         ];
         for storage in storages {
 
@@ -953,7 +956,10 @@ mod tests {
             Arc::new(ObjectStorage::new_in_memory_store()),
             Arc::new(ObjectStorage::new_local_store(Path::new(&temp_dir_name)).unwrap()),
             // Arc::new(ObjectStorage::new_s3_store_from_env("foo".to_string()).unwrap()),
-            Arc::new(ObjectStorage::new_s3_store_with_config("testbucket".to_string()).unwrap()),
+            Arc::new(
+                ObjectStorage::new_s3_store_with_config("testbucket".to_string())
+                    .unwrap(),
+            ),
         ];
         for storage in storages {
             let mut ds = Dataset::create(Arc::clone(&storage));
diff --git a/src/storage.rs b/src/storage.rs
index 39eef205..ae83e05d 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -55,18 +55,18 @@ impl ObjectStorage {
         })
     }
     pub fn new_s3_store_from_env(
-        bucket_name: String,
+        bucket_name: impl Into<String>,
     ) -> Result<ObjectStorage, StorageError> {
         use object_store::aws::AmazonS3Builder;
         let store = AmazonS3Builder::from_env()
-            .with_bucket_name(bucket_name)
+            .with_bucket_name(bucket_name.into())
             .build()
             .map_err(|err| StorageError::UrlParseError(Box::new(err)))?;
         Ok(ObjectStorage { store: Arc::new(store) })
     }
 
     pub fn new_s3_store_with_config(
-        bucket_name: String,
+        bucket_name: impl Into<String>,
     ) -> Result<ObjectStorage, StorageError> {
         use object_store::aws::AmazonS3Builder;
         let store = AmazonS3Builder::new()
@@ -75,14 +75,13 @@ impl ObjectStorage {
             .with_secret_access_key("minio123")
             .with_endpoint("http://localhost:9000")
             .with_allow_http(true)
-            .with_bucket_name(bucket_name)
+            .with_bucket_name(bucket_name.into())
             .build()
             .map_err(|err| StorageError::UrlParseError(Box::new(err)))?;
         Ok(ObjectStorage { store: Arc::new(store) })
     }
 
-    fn get_path(filetype: FileType, id: &ObjectId) -> Path {
-        let ObjectId(asu8) = id;
+    fn get_path(filetype: FileType, ObjectId(asu8): &ObjectId) -> Path {
         let prefix = filetype.get_prefix();
         // TODO: be careful about allocation here
         let path = format!("{}/{}", prefix, BASE64_URL_SAFE.encode(asu8));