Skip to content

Commit aa69742

Browse files
mohsen1mohsen1
authored
Merge pull request #11 from bodo-run/defaults
Fix #8: Config values now properly override default_value attributes
2 parents 011a148 + 210e012 commit aa69742

File tree

6 files changed

+1298
-50
lines changed

6 files changed

+1298
-50
lines changed

‎examples/basic/app-config.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
# Sample configuration file for the "basic" example.
2-
port: 8080
2+
port: 9090
33
database_url: "postgres://localhost:5432/mydb"
44
ignored_files:
55
- "*.log"

‎src/lib.rs‎

Lines changed: 224 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -278,51 +278,36 @@ fn generate_cli_field(field: &FieldInfo) -> TokenStream2 {
278278
};
279279

280280
if field.is_bool_type() {
281-
// Handle bool default_value "true"/"false"
282-
if let Some(ref dv) = field.arg_attrs.default_value {
283-
let is_true = dv.eq_ignore_ascii_case("true");
284-
let is_false = dv.eq_ignore_ascii_case("false");
285-
if !is_true && !is_false {
286-
let msg = format!(
287-
"For bool field, default_value must be \"true\" or \"false\", got {}",
288-
dv
289-
);
290-
return quote! {
291-
compile_error!(#msg);
292-
#ident: ()
293-
};
294-
}
295-
let bool_lit = if is_true { quote!(true) } else { quote!(false) };
296-
quote! {
297-
#[clap(long=#name_lit, #short_attr default_value_t=#bool_lit, #help_attr)]
298-
#ident: Option<bool>
299-
}
300-
} else {
301-
quote! {
302-
#[clap(long=#name_lit, #short_attr action=::clap::ArgAction::SetTrue, #help_attr)]
303-
#ident: Option<bool>
304-
}
281+
// Bool flags: do not set default_value_t; presence sets true
282+
quote! {
283+
#[clap(long=#name_lit, #short_attr action=::clap::ArgAction::SetTrue, #help_attr)]
284+
#ident: Option<bool>
285+
}
286+
} else if field.is_option_bool_type() {
287+
// Option<bool> flags: presence sets Some(true)
288+
quote! {
289+
#[clap(long=#name_lit, #short_attr action=::clap::ArgAction::SetTrue, #help_attr)]
290+
#ident: Option<bool>
305291
}
306292
} else {
307-
let dv_attr = if let Some(dv) = &field.arg_attrs.default_value {
308-
let dv_lit = LitStr::new(dv, Span::call_site());
309-
quote!(default_value=#dv_lit,)
310-
} else {
311-
quote!()
312-
};
293+
// No default values at CLI layer; treat CLI as presence-only
313294
let is_vec = field.is_vec_type();
314295
let multi = if is_vec {
315296
quote!(num_args = 1.., action = ::clap::ArgAction::Append,)
316297
} else {
317298
quote!()
318299
};
319-
let field_ty = {
300+
// For CLI, always wrap in Option unless already Option
301+
let field_ty = if field.is_option_type() {
302+
let t = &field.ty;
303+
quote!(#t)
304+
} else {
320305
let t = &field.ty;
321306
quote!(Option<#t>)
322307
};
323308

324309
quote! {
325-
#[clap(long=#name_lit, #short_attr #dv_attr #multi #help_attr)]
310+
#[clap(long=#name_lit, #short_attr #multi #help_attr)]
326311
#ident: #field_ty
327312
}
328313
}
@@ -341,49 +326,241 @@ fn generate_config_field(field: &FieldInfo) -> TokenStream2 {
341326
quote!()
342327
};
343328

329+
// Wrap non-Option types as Option<T> to detect presence in config
330+
// But don't double-wrap if already Option<T>
331+
let cfg_ty = if field.is_option_type() {
332+
quote!(#ty)
333+
} else {
334+
quote!(Option<#ty>)
335+
};
336+
344337
quote! {
345338
#rename_attr
346339
#[serde(default)]
347-
pub #ident: #ty
340+
pub #ident: #cfg_ty
348341
}
349342
}
350343

351344
/// Merge ephemeral CLI + ephemeral config => final
352345
fn unify_field(field: &FieldInfo) -> TokenStream2 {
353346
let ident = &field.ident;
347+
let ty = &field.ty;
348+
349+
// Helper: compute default Option<T> from attribute default_value
350+
let default_opt_expr = if field.is_bool_type() {
351+
if let Some(ref dv) = field.arg_attrs.default_value {
352+
let is_true = dv.eq_ignore_ascii_case("true");
353+
let is_false = dv.eq_ignore_ascii_case("false");
354+
if !is_true && !is_false {
355+
let msg = format!(
356+
"For bool field, default_value must be \"true\" or \"false\", got {}",
357+
dv
358+
);
359+
return quote! {
360+
compile_error!(#msg);
361+
#ident: Default::default()
362+
};
363+
}
364+
let bool_lit = if is_true { quote!(true) } else { quote!(false) };
365+
quote!(Some(#bool_lit))
366+
} else {
367+
quote!(None)
368+
}
369+
} else if field.is_vec_type() {
370+
// Support default_value for vectors as comma-separated strings
371+
if let Some(dv) = &field.arg_attrs.default_value {
372+
let dv_lit = LitStr::new(dv, Span::call_site());
373+
quote!(Some({
374+
let s: &str = #dv_lit;
375+
if s.is_empty() {
376+
vec![]
377+
} else {
378+
s.split(',').map(|x| x.trim().to_string()).collect()
379+
}
380+
}))
381+
} else {
382+
quote!(None)
383+
}
384+
} else {
385+
// Non-bool scalar types: parse from string if provided
386+
if let Some(dv) = &field.arg_attrs.default_value {
387+
let dv_lit = LitStr::new(dv, Span::call_site());
388+
// We'll handle the parsing at the use site
389+
quote!(Some(#dv_lit))
390+
} else {
391+
quote!(None)
392+
}
393+
};
394+
354395
match field.arg_attrs.availability {
355396
FieldAvailability::CliOnly => {
356397
if field.is_vec_type() {
357-
quote!(#ident: cli.#ident.unwrap_or_default())
398+
// Apply default_value if no CLI input
399+
if let Some(dv) = &field.arg_attrs.default_value {
400+
let dv_lit = LitStr::new(dv, Span::call_site());
401+
quote!(#ident: cli.#ident.unwrap_or_else(|| {
402+
let s: &str = #dv_lit;
403+
if s.is_empty() {
404+
vec![]
405+
} else {
406+
s.split(',').map(|x| x.trim().to_string()).collect()
407+
}
408+
}))
409+
} else {
410+
quote!(#ident: cli.#ident.unwrap_or_default())
411+
}
358412
} else if field.is_bool_type() {
359413
quote!(#ident: cli.#ident.unwrap_or(false))
414+
} else if field.is_option_type() || field.is_option_bool_type() {
415+
quote!(#ident: cli.#ident)
360416
} else {
361-
quote!(#ident: cli.#ident.unwrap_or_default())
417+
// Apply default_value for scalar types if no CLI input
418+
if let Some(dv) = &field.arg_attrs.default_value {
419+
let dv_lit = LitStr::new(dv, Span::call_site());
420+
// String special-case
421+
let is_string = matches!(ty, syn::Type::Path(ref tp) if tp.path.segments.last().map(|s| s.ident == "String").unwrap_or(false));
422+
if is_string {
423+
quote!(#ident: cli.#ident.unwrap_or_else(|| #dv_lit.to_string()))
424+
} else {
425+
quote!(#ident: cli.#ident.unwrap_or_else(|| ::std::str::FromStr::from_str(#dv_lit).ok().unwrap_or_default()))
426+
}
427+
} else {
428+
quote!(#ident: cli.#ident.unwrap_or_default())
429+
}
362430
}
363431
}
364432
FieldAvailability::ConfigOnly => {
365-
quote!(#ident: ephemeral_cfg.#ident)
433+
if field.is_vec_type() {
434+
// Apply default_value if no config value
435+
if let Some(dv) = &field.arg_attrs.default_value {
436+
let dv_lit = LitStr::new(dv, Span::call_site());
437+
quote!(#ident: ephemeral_cfg.#ident.unwrap_or_else(|| {
438+
let s: &str = #dv_lit;
439+
if s.is_empty() {
440+
vec![]
441+
} else {
442+
s.split(',').map(|x| x.trim().to_string()).collect()
443+
}
444+
}))
445+
} else {
446+
quote!(#ident: ephemeral_cfg.#ident.unwrap_or_default())
447+
}
448+
} else if field.is_bool_type() {
449+
quote!(#ident: ephemeral_cfg.#ident.unwrap_or(false))
450+
} else if field.is_option_type() || field.is_option_bool_type() {
451+
quote!(#ident: ephemeral_cfg.#ident)
452+
} else {
453+
quote!(#ident: ephemeral_cfg.#ident.unwrap_or_default())
454+
}
366455
}
367456
FieldAvailability::CliAndConfig => {
368457
if field.is_vec_type() {
369458
match field.arg_attrs.multi_value_behavior {
370-
MultiValueBehavior::Extend => quote! {
371-
#ident: {
372-
let mut merged = ephemeral_cfg.#ident.clone();
373-
if let Some(cli_vec) = cli.#ident {
374-
merged.extend(cli_vec);
459+
MultiValueBehavior::Extend => {
460+
// For extend: merge config + CLI, with default as fallback
461+
if let Some(dv) = &field.arg_attrs.default_value {
462+
let dv_lit = LitStr::new(dv, Span::call_site());
463+
quote! {
464+
#ident: {
465+
let default_vec = {
466+
let s: &str = #dv_lit;
467+
if s.is_empty() {
468+
vec![]
469+
} else {
470+
s.split(',').map(|x| x.trim().to_string()).collect()
471+
}
472+
};
473+
let mut merged = ephemeral_cfg.#ident.unwrap_or(default_vec);
474+
if let Some(cli_vec) = cli.#ident {
475+
merged.extend(cli_vec);
476+
}
477+
merged
478+
}
479+
}
480+
} else {
481+
quote! {
482+
#ident: {
483+
let mut merged = ephemeral_cfg.#ident.unwrap_or_default();
484+
if let Some(cli_vec) = cli.#ident {
485+
merged.extend(cli_vec);
486+
}
487+
merged
488+
}
375489
}
376-
merged
377490
}
378491
},
379-
MultiValueBehavior::Overwrite => quote! {
380-
#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident.clone())
381-
},
492+
MultiValueBehavior::Overwrite => {
493+
// For overwrite: CLI > config > default
494+
if let Some(dv) = &field.arg_attrs.default_value {
495+
let dv_lit = LitStr::new(dv, Span::call_site());
496+
quote! {
497+
#ident: cli.#ident.or(ephemeral_cfg.#ident).unwrap_or_else(|| {
498+
let s: &str = #dv_lit;
499+
if s.is_empty() {
500+
vec![]
501+
} else {
502+
s.split(',').map(|x| x.trim().to_string()).collect()
503+
}
504+
})
505+
}
506+
} else {
507+
quote! {
508+
#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident.unwrap_or_default())
509+
}
510+
}
511+
}
382512
}
383513
} else if field.is_bool_type() {
384-
quote!(#ident: cli.#ident.unwrap_or(ephemeral_cfg.#ident))
514+
// Final bool (is_bool_type() already excludes Option<bool>)
515+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident).or(#default_opt_expr).unwrap_or(false))
516+
} else if field.is_option_bool_type() {
517+
// Handle Option<bool> specifically
518+
if let Some(ref dv) = field.arg_attrs.default_value {
519+
let is_true = dv.eq_ignore_ascii_case("true");
520+
let is_false = dv.eq_ignore_ascii_case("false");
521+
if is_true {
522+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident).or(Some(true)))
523+
} else if is_false {
524+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident).or(Some(false)))
525+
} else {
526+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident))
527+
}
528+
} else {
529+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident))
530+
}
531+
} else if field.is_option_type() {
532+
// Final Option<T>: precedence CLI > config > default
533+
// For non-bool defaults, we parse at use site below
534+
if let Some(dv) = &field.arg_attrs.default_value {
535+
let dv_lit = LitStr::new(dv, Span::call_site());
536+
if let Some(inner) = field.option_inner_type() {
537+
// String special-case
538+
let is_string = matches!(inner, syn::Type::Path(ref tp) if tp.path.segments.last().map(|s| s.ident == "String").unwrap_or(false));
539+
if is_string {
540+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident).or(Some(#dv_lit.to_string())))
541+
} else {
542+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident).or(::std::str::FromStr::from_str(#dv_lit).ok()))
543+
}
544+
} else {
545+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident))
546+
}
547+
} else {
548+
quote!(#ident: cli.#ident.or(ephemeral_cfg.#ident))
549+
}
385550
} else {
386-
quote!(#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident))
551+
// Final T: precedence CLI > config > default_value > Default::default()
552+
if let Some(dv) = &field.arg_attrs.default_value {
553+
let dv_lit = LitStr::new(dv, Span::call_site());
554+
// String special-case
555+
let is_string = matches!(ty, syn::Type::Path(ref tp) if tp.path.segments.last().map(|s| s.ident == "String").unwrap_or(false));
556+
if is_string {
557+
quote!(#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident.unwrap_or_else(|| #dv_lit.to_string())))
558+
} else {
559+
quote!(#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident.unwrap_or_else(|| ::std::str::FromStr::from_str(#dv_lit).ok().unwrap_or_default())))
560+
}
561+
} else {
562+
quote!(#ident: cli.#ident.unwrap_or_else(|| ephemeral_cfg.#ident.unwrap_or_default()))
563+
}
387564
}
388565
}
389566
FieldAvailability::Internal => {

‎src/parse_attrs.rs‎

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,12 @@ pub struct FieldInfo {
4444
pub arg_attrs: ArgAttributes,
4545
}
4646
impl FieldInfo {
47-
// e.g. "bool" => is_bool_type
47+
// e.g. "bool" => is_bool_type (but not Option<bool>)
4848
pub fn is_bool_type(&self) -> bool {
49+
// First check it's not Option<bool>
50+
if self.is_option_type() {
51+
return false;
52+
}
4953
if let syn::Type::Path(tp) = &self.ty {
5054
if let Some(seg) = tp.path.segments.last() {
5155
return seg.ident == "bool";
@@ -62,6 +66,41 @@ impl FieldInfo {
6266
}
6367
false
6468
}
69+
/// Returns true if the field type is Option<..>
70+
pub fn is_option_type(&self) -> bool {
71+
if let syn::Type::Path(tp) = &self.ty {
72+
if let Some(seg) = tp.path.segments.last() {
73+
return seg.ident == "Option";
74+
}
75+
}
76+
false
77+
}
78+
/// If the field type is Option<Inner>, returns Some(Inner).
79+
pub fn option_inner_type(&self) -> Option<syn::Type> {
80+
if let syn::Type::Path(tp) = &self.ty {
81+
if let Some(seg) = tp.path.segments.last() {
82+
if seg.ident == "Option" {
83+
if let syn::PathArguments::AngleBracketed(args) = &seg.arguments {
84+
for ga in &args.args {
85+
if let syn::GenericArgument::Type(t) = ga {
86+
return Some(t.clone());
87+
}
88+
}
89+
}
90+
}
91+
}
92+
}
93+
None
94+
}
95+
/// Returns true if the field type is Option<bool>
96+
pub fn is_option_bool_type(&self) -> bool {
97+
if let Some(syn::Type::Path(tp)) = self.option_inner_type().as_ref() {
98+
if let Some(seg) = tp.path.segments.last() {
99+
return seg.ident == "bool";
100+
}
101+
}
102+
false
103+
}
65104
}
66105

67106
/// Parse struct-level: #[config_file_name(...)] / #[config_file_formats(...)]

0 commit comments

Comments
 (0)