jonathanc-n commented on code in PR #1873:
URL: https://github.com/apache/iceberg-rust/pull/1873#discussion_r2558554976
##########
crates/catalog/rest/src/catalog.rs:
##########
@@ -79,56 +81,55 @@ impl CatalogBuilder for RestCatalogBuilder {
name: impl Into<String>,
props: HashMap<String, String>,
) -> impl Future<Output = Result<Self::C>> + Send {
- self.0.name = Some(name.into());
+ self.name = Some(name.into());
if props.contains_key(REST_CATALOG_PROP_URI) {
- self.0.uri = props
- .get(REST_CATALOG_PROP_URI)
- .cloned()
- .unwrap_or_default();
+ self.uri = props.get(REST_CATALOG_PROP_URI).cloned();
}
if props.contains_key(REST_CATALOG_PROP_WAREHOUSE) {
- self.0.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned()
+ self.warehouse = props.get(REST_CATALOG_PROP_WAREHOUSE).cloned();
}
// Collect other remaining properties
- self.0.props = props
+ self.props = props
.into_iter()
.filter(|(k, _)| k != REST_CATALOG_PROP_URI && k !=
REST_CATALOG_PROP_WAREHOUSE)
.collect();
- let result = {
- if self.0.name.is_none() {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Catalog name is required",
- ))
- } else if self.0.uri.is_empty() {
- Err(Error::new(
+ async move {
+ let name = self
+ .name
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog
name is required"))?;
+
+ let uri = self
+ .uri
+ .ok_or_else(|| Error::new(ErrorKind::DataInvalid, "Catalog uri
is required"))?;
+
+ if uri.is_empty() {
+ return Err(Error::new(
ErrorKind::DataInvalid,
- "Catalog uri is required",
- ))
- } else {
- Ok(RestCatalog::new(self.0))
+ "Catalog uri cannot be empty",
+ ));
}
- };
- std::future::ready(result)
- }
-}
+ let config = RestCatalogConfig {
+ name: Some(name),
+ uri,
+ warehouse: self.warehouse,
+ props: self.props,
+ client: self.client,
+ };
-impl RestCatalogBuilder {
- /// Configures the catalog with a custom HTTP client.
- pub fn with_client(mut self, client: Client) -> Self {
- self.0.client = Some(client);
- self
+ Ok(RestCatalog::new(config))
+ }
}
}
/// Rest catalog configuration.
#[derive(Clone, Debug, TypedBuilder)]
Review Comment:
Just some questions for this one. Would this be breaking? Should I change
the builder functionality for RestCatalog to be similar to the other catalogs
(ex. using the load function.? Do you know why the builder for this one is
different from the other catalogs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
