chitralverma commented on code in PR #7883:
URL: https://github.com/apache/opendal/pull/7883#discussion_r3544219417
##########
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:
All fields carrying a `@default` marker are already declared `Option<T>` in
core (e.g. s3/gcs `root`, gcs `endpoint`), so they are already treated as
optional. There are currently no required (non-`Option`, non-`bool`) fields
with a documented default, so this does not change any generated signature
today. Reasonable to also honour `default_value` here as a future-proofing
follow-up.
##########
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:
Intentional. The getter type is deliberately kept equal to the constructor
parameter (`bool | None`) so that a value read back from a config can be passed
straight into another config without a type mismatch. A returned `bool`
satisfies `bool | None`, so the annotation is not incorrect, just wider than
strictly necessary.
--
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]
