Copilot commented on code in PR #7883:
URL: https://github.com/apache/opendal/pull/7883#discussion_r3543767561
##########
dev/src/generate/python.rs:
##########
@@ -244,3 +298,280 @@ fn make_python_type(ty: ViaDeserialize<ConfigType>) ->
Result<String, minijinja:
}
.to_string())
}
+
+/// Classifies a config field to drive codegen.
+#[derive(Clone, Copy, PartialEq)]
+enum FieldKind {
+ Str,
+ StrRequired,
+ Path,
+ Bool,
+ Num,
+ Vec,
+ Map,
+ Duration,
+ /// A core type the parser can't map (e.g. an enum); passed through as a
string.
+ Other,
+}
+
+fn field_kind(field: &Config) -> FieldKind {
+ match field.value {
+ ConfigType::Bool => FieldKind::Bool,
+ ConfigType::Usize | ConfigType::U64 | ConfigType::I64 |
ConfigType::U32 | ConfigType::U16 => {
+ FieldKind::Num
+ }
+ ConfigType::Vec => FieldKind::Vec,
+ ConfigType::HashMap => FieldKind::Map,
+ ConfigType::Duration => FieldKind::Duration,
+ ConfigType::String => {
+ if is_opaque_string_fallback(&field.name) {
+ FieldKind::Other
+ } else if is_path_like_field(&field.name) {
+ FieldKind::Path
+ } else if field.optional {
+ FieldKind::Str
+ } else {
+ FieldKind::StrRequired
+ }
+ }
+ }
+}
+
+/// Config fields whose core type is a string-parsed enum, not a `String`.
+fn is_opaque_string_fallback(field: &str) -> bool {
+ matches!(field, "repo_type" | "download_mode")
+}
+
+fn int_type(ty: ConfigType) -> &'static str {
+ match ty {
+ ConfigType::Usize => "usize",
+ ConfigType::U64 => "u64",
+ ConfigType::I64 => "i64",
+ ConfigType::U32 => "u32",
+ ConfigType::U16 => "u16",
+ _ => "usize",
+ }
+}
+
+/// The Rust `#[new]` parameter type, chosen so introspection renders the exact
+/// Python type.
+fn config_param_type(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ Ok(match field_kind(&field) {
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other =>
"Option<String>".to_string(),
+ FieldKind::StrRequired => "String".to_string(),
+ FieldKind::Path => "Option<crate::PyPath>".to_string(),
+ FieldKind::Bool => "Option<bool>".to_string(),
+ FieldKind::Num => format!("Option<{}>", int_type(field.value)),
+ FieldKind::Vec => "Option<Vec<String>>".to_string(),
+ FieldKind::Map => "Option<std::collections::HashMap<String,
String>>".to_string(),
+ })
+}
+
+/// The `#[new]` signature parameter, defaulting optional fields to `None`.
+fn config_new_param(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ if field_is_optional(&field) {
+ Ok(format!("{} = None", field.name))
+ } else {
+ Ok(field.name.to_string())
+ }
+}
+
+/// Insert a `#[new]` parameter into the option map handed to `from_iter`.
+fn config_to_opts(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => format!("opts.insert(\"{key}\".to_string(),
{name});"),
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other => {
+ format!("if let Some(v) = {name} {{
opts.insert(\"{key}\".to_string(), v); }}")
+ }
+ FieldKind::Path => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.into_string()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(), if
v {{ \"true\" }} else {{ \"false\" }}.to_string()); }}"
+ ),
+ FieldKind::Num => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.to_string()); }}"
+ ),
+ FieldKind::Vec => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.join(\",\")); }}"
+ ),
+ // Map fields are set on the config directly after `from_iter`.
+ FieldKind::Map => String::new(),
+ })
+}
+
+/// Set a map-valued field on the core config after `from_iter` (map values
+/// cannot pass through the flat option map). No-op for other kinds.
+fn config_assign_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::Map => format!("if let Some(v) = {name} {{ cfg.{name} =
Some(v); }}"),
+ _ => String::new(),
+ })
+}
+
+/// A getter (with doc comment) for a config field. Opaque fields have none.
+fn config_getter(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let kind = field_kind(&field);
+
+ if kind == FieldKind::Other {
+ return Ok(String::new());
+ }
+
+ let body = match kind {
+ FieldKind::StrRequired => {
+ format!("#[getter]\n fn {name}(&self) -> String {{
self.0.{name}.clone() }}")
+ }
+ FieldKind::Str => {
+ format!("#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.clone() }}")
+ }
+ // `PyPath`/`Option<bool>` keep the getter type equal to the
constructor.
+ FieldKind::Path => format!(
+ "#[getter]\n fn {name}(&self) -> Option<crate::PyPath> {{
self.0.{name}.clone().map(crate::PyPath::from) }}"
+ ),
+ FieldKind::Bool => {
+ format!("#[getter]\n fn {name}(&self) -> Option<bool> {{
Some(self.0.{name}) }}")
+ }
Review Comment:
Bool config getters currently return `Option<bool>` but always wrap the
underlying core `bool` in `Some(...)`, so `None` is never actually possible.
This makes the generated stubs misleading (`bool | None`) for properties that
are always `bool`.
##########
dev/src/generate/python.rs:
##########
@@ -15,25 +15,69 @@
// specific language governing permissions and limitations
// under the License.
-use crate::generate::parser::{ConfigType, Services, sorted_services};
+use crate::generate::parser::{Config, ConfigType, Service, Services,
sorted_services};
use anyhow::Result;
use minijinja::value::ViaDeserialize;
use minijinja::{Environment, context};
use std::fs;
use std::path::PathBuf;
+/// Path-like fields, widened to accept `os.PathLike` in addition to `str`.
+fn is_path_like_field(name: &str) -> bool {
+ matches!(name, "root" | "atomic_write_dir")
+}
+
+/// Whether a field is keyword-optional in Python (optional core fields and
bools).
+fn field_is_optional(field: &Config) -> bool {
+ field.optional || field.value == ConfigType::Bool
+}
Review Comment:
`field_is_optional` only considers `Option<T>` fields and bools, but the
parser also provides `default_value` markers (`<!-- @default ... -->`).
Ignoring defaults makes fields with documented defaults appear required in
generated `#[new]` signatures, which is inconsistent with their docs and how
configs deserialize defaults.
##########
dev/src/generate/python.rs:
##########
@@ -244,3 +298,280 @@ fn make_python_type(ty: ViaDeserialize<ConfigType>) ->
Result<String, minijinja:
}
.to_string())
}
+
+/// Classifies a config field to drive codegen.
+#[derive(Clone, Copy, PartialEq)]
+enum FieldKind {
+ Str,
+ StrRequired,
+ Path,
+ Bool,
+ Num,
+ Vec,
+ Map,
+ Duration,
+ /// A core type the parser can't map (e.g. an enum); passed through as a
string.
+ Other,
+}
+
+fn field_kind(field: &Config) -> FieldKind {
+ match field.value {
+ ConfigType::Bool => FieldKind::Bool,
+ ConfigType::Usize | ConfigType::U64 | ConfigType::I64 |
ConfigType::U32 | ConfigType::U16 => {
+ FieldKind::Num
+ }
+ ConfigType::Vec => FieldKind::Vec,
+ ConfigType::HashMap => FieldKind::Map,
+ ConfigType::Duration => FieldKind::Duration,
+ ConfigType::String => {
+ if is_opaque_string_fallback(&field.name) {
+ FieldKind::Other
+ } else if is_path_like_field(&field.name) {
+ FieldKind::Path
+ } else if field.optional {
+ FieldKind::Str
+ } else {
+ FieldKind::StrRequired
+ }
+ }
+ }
+}
+
+/// Config fields whose core type is a string-parsed enum, not a `String`.
+fn is_opaque_string_fallback(field: &str) -> bool {
+ matches!(field, "repo_type" | "download_mode")
+}
+
+fn int_type(ty: ConfigType) -> &'static str {
+ match ty {
+ ConfigType::Usize => "usize",
+ ConfigType::U64 => "u64",
+ ConfigType::I64 => "i64",
+ ConfigType::U32 => "u32",
+ ConfigType::U16 => "u16",
+ _ => "usize",
+ }
+}
+
+/// The Rust `#[new]` parameter type, chosen so introspection renders the exact
+/// Python type.
+fn config_param_type(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ Ok(match field_kind(&field) {
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other =>
"Option<String>".to_string(),
+ FieldKind::StrRequired => "String".to_string(),
+ FieldKind::Path => "Option<crate::PyPath>".to_string(),
+ FieldKind::Bool => "Option<bool>".to_string(),
+ FieldKind::Num => format!("Option<{}>", int_type(field.value)),
+ FieldKind::Vec => "Option<Vec<String>>".to_string(),
+ FieldKind::Map => "Option<std::collections::HashMap<String,
String>>".to_string(),
+ })
+}
+
+/// The `#[new]` signature parameter, defaulting optional fields to `None`.
+fn config_new_param(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ if field_is_optional(&field) {
+ Ok(format!("{} = None", field.name))
+ } else {
+ Ok(field.name.to_string())
+ }
+}
+
+/// Insert a `#[new]` parameter into the option map handed to `from_iter`.
+fn config_to_opts(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => format!("opts.insert(\"{key}\".to_string(),
{name});"),
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other => {
+ format!("if let Some(v) = {name} {{
opts.insert(\"{key}\".to_string(), v); }}")
+ }
+ FieldKind::Path => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.into_string()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(), if
v {{ \"true\" }} else {{ \"false\" }}.to_string()); }}"
+ ),
+ FieldKind::Num => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.to_string()); }}"
+ ),
+ FieldKind::Vec => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.join(\",\")); }}"
+ ),
+ // Map fields are set on the config directly after `from_iter`.
+ FieldKind::Map => String::new(),
+ })
+}
+
+/// Set a map-valued field on the core config after `from_iter` (map values
+/// cannot pass through the flat option map). No-op for other kinds.
+fn config_assign_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::Map => format!("if let Some(v) = {name} {{ cfg.{name} =
Some(v); }}"),
+ _ => String::new(),
+ })
+}
+
+/// A getter (with doc comment) for a config field. Opaque fields have none.
+fn config_getter(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let kind = field_kind(&field);
+
+ if kind == FieldKind::Other {
+ return Ok(String::new());
+ }
+
+ let body = match kind {
+ FieldKind::StrRequired => {
+ format!("#[getter]\n fn {name}(&self) -> String {{
self.0.{name}.clone() }}")
+ }
+ FieldKind::Str => {
+ format!("#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.clone() }}")
+ }
+ // `PyPath`/`Option<bool>` keep the getter type equal to the
constructor.
+ FieldKind::Path => format!(
+ "#[getter]\n fn {name}(&self) -> Option<crate::PyPath> {{
self.0.{name}.clone().map(crate::PyPath::from) }}"
+ ),
+ FieldKind::Bool => {
+ format!("#[getter]\n fn {name}(&self) -> Option<bool> {{
Some(self.0.{name}) }}")
+ }
+ FieldKind::Num => {
+ let ty = int_type(field.value);
+ if field.optional {
+ format!("#[getter]\n fn {name}(&self) -> Option<{ty}> {{
self.0.{name} }}")
+ } else {
+ format!("#[getter]\n fn {name}(&self) -> {ty} {{
self.0.{name} }}")
+ }
+ }
+ FieldKind::Vec => {
+ format!(
+ "#[getter]\n fn {name}(&self) -> Option<Vec<String>> {{
self.0.{name}.clone() }}"
+ )
+ }
+ FieldKind::Map => format!(
+ "#[getter]\n fn {name}(&self) ->
Option<std::collections::HashMap<String, String>> {{ self.0.{name}.clone() }}"
+ ),
+ FieldKind::Duration => format!(
+ "#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.map(|d| format!(\"{{}}s\", d.as_secs())) }}"
+ ),
Review Comment:
Duration getters format values as whole seconds (`"{secs}s"`), which
truncates sub-second durations (e.g. `500ms` becomes `0s`). This breaks
round-tripping for configs that include `Duration` fields and can change
behavior after pickling/unpickling or when users inspect the config value.
##########
dev/src/generate/python.rs:
##########
@@ -244,3 +298,280 @@ fn make_python_type(ty: ViaDeserialize<ConfigType>) ->
Result<String, minijinja:
}
.to_string())
}
+
+/// Classifies a config field to drive codegen.
+#[derive(Clone, Copy, PartialEq)]
+enum FieldKind {
+ Str,
+ StrRequired,
+ Path,
+ Bool,
+ Num,
+ Vec,
+ Map,
+ Duration,
+ /// A core type the parser can't map (e.g. an enum); passed through as a
string.
+ Other,
+}
+
+fn field_kind(field: &Config) -> FieldKind {
+ match field.value {
+ ConfigType::Bool => FieldKind::Bool,
+ ConfigType::Usize | ConfigType::U64 | ConfigType::I64 |
ConfigType::U32 | ConfigType::U16 => {
+ FieldKind::Num
+ }
+ ConfigType::Vec => FieldKind::Vec,
+ ConfigType::HashMap => FieldKind::Map,
+ ConfigType::Duration => FieldKind::Duration,
+ ConfigType::String => {
+ if is_opaque_string_fallback(&field.name) {
+ FieldKind::Other
+ } else if is_path_like_field(&field.name) {
+ FieldKind::Path
+ } else if field.optional {
+ FieldKind::Str
+ } else {
+ FieldKind::StrRequired
+ }
+ }
+ }
+}
+
+/// Config fields whose core type is a string-parsed enum, not a `String`.
+fn is_opaque_string_fallback(field: &str) -> bool {
+ matches!(field, "repo_type" | "download_mode")
+}
+
+fn int_type(ty: ConfigType) -> &'static str {
+ match ty {
+ ConfigType::Usize => "usize",
+ ConfigType::U64 => "u64",
+ ConfigType::I64 => "i64",
+ ConfigType::U32 => "u32",
+ ConfigType::U16 => "u16",
+ _ => "usize",
+ }
+}
+
+/// The Rust `#[new]` parameter type, chosen so introspection renders the exact
+/// Python type.
+fn config_param_type(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ Ok(match field_kind(&field) {
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other =>
"Option<String>".to_string(),
+ FieldKind::StrRequired => "String".to_string(),
+ FieldKind::Path => "Option<crate::PyPath>".to_string(),
+ FieldKind::Bool => "Option<bool>".to_string(),
+ FieldKind::Num => format!("Option<{}>", int_type(field.value)),
+ FieldKind::Vec => "Option<Vec<String>>".to_string(),
+ FieldKind::Map => "Option<std::collections::HashMap<String,
String>>".to_string(),
+ })
+}
+
+/// The `#[new]` signature parameter, defaulting optional fields to `None`.
+fn config_new_param(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ if field_is_optional(&field) {
+ Ok(format!("{} = None", field.name))
+ } else {
+ Ok(field.name.to_string())
+ }
+}
+
+/// Insert a `#[new]` parameter into the option map handed to `from_iter`.
+fn config_to_opts(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => format!("opts.insert(\"{key}\".to_string(),
{name});"),
+ FieldKind::Str | FieldKind::Duration | FieldKind::Other => {
+ format!("if let Some(v) = {name} {{
opts.insert(\"{key}\".to_string(), v); }}")
+ }
+ FieldKind::Path => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.into_string()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(), if
v {{ \"true\" }} else {{ \"false\" }}.to_string()); }}"
+ ),
+ FieldKind::Num => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.to_string()); }}"
+ ),
+ FieldKind::Vec => format!(
+ "if let Some(v) = {name} {{ opts.insert(\"{key}\".to_string(),
v.join(\",\")); }}"
+ ),
+ // Map fields are set on the config directly after `from_iter`.
+ FieldKind::Map => String::new(),
+ })
+}
+
+/// Set a map-valued field on the core config after `from_iter` (map values
+/// cannot pass through the flat option map). No-op for other kinds.
+fn config_assign_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::Map => format!("if let Some(v) = {name} {{ cfg.{name} =
Some(v); }}"),
+ _ => String::new(),
+ })
+}
+
+/// A getter (with doc comment) for a config field. Opaque fields have none.
+fn config_getter(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let kind = field_kind(&field);
+
+ if kind == FieldKind::Other {
+ return Ok(String::new());
+ }
+
+ let body = match kind {
+ FieldKind::StrRequired => {
+ format!("#[getter]\n fn {name}(&self) -> String {{
self.0.{name}.clone() }}")
+ }
+ FieldKind::Str => {
+ format!("#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.clone() }}")
+ }
+ // `PyPath`/`Option<bool>` keep the getter type equal to the
constructor.
+ FieldKind::Path => format!(
+ "#[getter]\n fn {name}(&self) -> Option<crate::PyPath> {{
self.0.{name}.clone().map(crate::PyPath::from) }}"
+ ),
+ FieldKind::Bool => {
+ format!("#[getter]\n fn {name}(&self) -> Option<bool> {{
Some(self.0.{name}) }}")
+ }
+ FieldKind::Num => {
+ let ty = int_type(field.value);
+ if field.optional {
+ format!("#[getter]\n fn {name}(&self) -> Option<{ty}> {{
self.0.{name} }}")
+ } else {
+ format!("#[getter]\n fn {name}(&self) -> {ty} {{
self.0.{name} }}")
+ }
+ }
+ FieldKind::Vec => {
+ format!(
+ "#[getter]\n fn {name}(&self) -> Option<Vec<String>> {{
self.0.{name}.clone() }}"
+ )
+ }
+ FieldKind::Map => format!(
+ "#[getter]\n fn {name}(&self) ->
Option<std::collections::HashMap<String, String>> {{ self.0.{name}.clone() }}"
+ ),
+ FieldKind::Duration => format!(
+ "#[getter]\n fn {name}(&self) -> Option<String> {{
self.0.{name}.map(|d| format!(\"{{}}s\", d.as_secs())) }}"
+ ),
+ FieldKind::Other => unreachable!(),
+ };
+
+ let doc = config_field_doc(field)?;
+ if doc.is_empty() {
+ Ok(body)
+ } else {
+ Ok(format!("{doc}\n {body}"))
+ }
+}
+
+/// Insert a field into the flat option map used for `repr`/pickle.
+fn config_to_map(field: ViaDeserialize<Config>) -> Result<String,
minijinja::Error> {
+ let name = &field.name;
+ let key = &field.name;
+ Ok(match field_kind(&field) {
+ FieldKind::StrRequired => {
+ format!("map.insert(\"{key}\".to_string(),
self.0.{name}.clone());")
+ }
+ FieldKind::Str | FieldKind::Path => format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.clone()); }}"
+ ),
+ FieldKind::Bool => format!(
+ "map.insert(\"{key}\".to_string(), if self.0.{name} {{ \"true\" }}
else {{ \"false\" }}.to_string());"
+ ),
+ FieldKind::Num => {
+ if field.optional {
+ format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.to_string()); }}"
+ )
+ } else {
+ format!("map.insert(\"{key}\".to_string(),
self.0.{name}.to_string());")
+ }
+ }
+ FieldKind::Vec => format!(
+ "if let Some(v) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), v.join(\",\")); }}"
+ ),
+ FieldKind::Duration => format!(
+ "if let Some(d) = &self.0.{name} {{
map.insert(\"{key}\".to_string(), format!(\"{{}}s\", d.as_secs())); }}"
+ ),
Review Comment:
`config_to_map` serializes `Duration` fields using `as_secs()` and appends
`s`, which truncates sub-second precision (e.g. `500ms` -> `0s`). Since the
operator pickle payload uses this flat map, this can silently change config
semantics on pickle round-trip.
--
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]
