-
Notifications
You must be signed in to change notification settings - Fork 233
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for unpacked TypedDict
to type hint variadic keyword arguments in ArgumentsValidator
#1451
base: main
Are you sure you want to change the base?
Add support for unpacked TypedDict
to type hint variadic keyword arguments in ArgumentsValidator
#1451
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
use std::str::FromStr; | ||
|
||
use pyo3::intern; | ||
use pyo3::prelude::*; | ||
use pyo3::types::{PyDict, PyList, PyString, PyTuple}; | ||
|
@@ -15,6 +17,27 @@ use crate::tools::SchemaDict; | |
use super::validation_state::ValidationState; | ||
use super::{build_validator, BuildValidator, CombinedValidator, DefinitionsBuilder, Validator}; | ||
|
||
#[derive(Debug, PartialEq)] | ||
enum VarKwargsMode { | ||
Uniform, | ||
UnpackedTypedDict, | ||
} | ||
|
||
impl FromStr for VarKwargsMode { | ||
type Err = PyErr; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
match s { | ||
"uniform" => Ok(Self::Uniform), | ||
"unpacked-typed-dict" => Ok(Self::UnpackedTypedDict), | ||
s => py_schema_err!( | ||
"Invalid var_kwargs mode: `{}`, expected `uniform` or `unpacked-typed-dict`", | ||
s | ||
), | ||
} | ||
} | ||
} | ||
|
||
#[derive(Debug)] | ||
struct Parameter { | ||
positional: bool, | ||
|
@@ -29,6 +52,7 @@ pub struct ArgumentsValidator { | |
parameters: Vec<Parameter>, | ||
positional_params_count: usize, | ||
var_args_validator: Option<Box<CombinedValidator>>, | ||
var_kwargs_mode: VarKwargsMode, | ||
var_kwargs_validator: Option<Box<CombinedValidator>>, | ||
loc_by_alias: bool, | ||
extra: ExtraBehavior, | ||
|
@@ -117,17 +141,31 @@ impl BuildValidator for ArgumentsValidator { | |
}); | ||
} | ||
|
||
let py_var_kwargs_mode: Bound<PyString> = schema | ||
.get_as(intern!(py, "var_kwargs_mode"))? | ||
.unwrap_or_else(|| PyString::new_bound(py, "single")); | ||
|
||
let var_kwargs_mode = VarKwargsMode::from_str(py_var_kwargs_mode.to_str()?)?; | ||
let var_kwargs_validator = match schema.get_item(intern!(py, "var_kwargs_schema"))? { | ||
Some(v) => Some(Box::new(build_validator(&v, config, definitions)?)), | ||
None => None, | ||
}; | ||
Viicos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if var_kwargs_mode == VarKwargsMode::UnpackedTypedDict && var_kwargs_validator.is_none() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we should also check that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need to, given that you have the conditional checks in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making it a But I wonder, are there cases where it can be wrapped in a function-after validator (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The thing is (as described here)
only config can be attached to typed dicts iirc, and it still results in a typed dict schema. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could have #[derive(Debug, PartialEq)]
enum VarKwargsMode {
Uniform(CombinedValidator),
UnpackedTypedDict(TypedDictValidator),
} ... and replace the |
||
return py_schema_err!( | ||
"`var_kwargs_schema` must be specified when `var_kwargs_mode` is `'unpacked-typed-dict'`" | ||
); | ||
} | ||
|
||
Ok(Self { | ||
parameters, | ||
positional_params_count, | ||
var_args_validator: match schema.get_item(intern!(py, "var_args_schema"))? { | ||
Some(v) => Some(Box::new(build_validator(&v, config, definitions)?)), | ||
None => None, | ||
}, | ||
Viicos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var_kwargs_validator: match schema.get_item(intern!(py, "var_kwargs_schema"))? { | ||
Some(v) => Some(Box::new(build_validator(&v, config, definitions)?)), | ||
None => None, | ||
}, | ||
var_kwargs_mode, | ||
var_kwargs_validator, | ||
loc_by_alias: config.get_as(intern!(py, "loc_by_alias"))?.unwrap_or(true), | ||
extra: ExtraBehavior::from_schema_or_config(py, schema, config, ExtraBehavior::Forbid)?, | ||
} | ||
|
@@ -258,6 +296,8 @@ impl Validator for ArgumentsValidator { | |
// if there are kwargs check any that haven't been processed yet | ||
if let Some(kwargs) = args.kwargs() { | ||
if kwargs.len() > used_kwargs.len() { | ||
let remaining_kwargs = PyDict::new_bound(py); | ||
|
||
for result in kwargs.iter() { | ||
let (raw_key, value) = result?; | ||
let either_str = match raw_key | ||
|
@@ -278,30 +318,55 @@ impl Validator for ArgumentsValidator { | |
Err(err) => return Err(err), | ||
}; | ||
if !used_kwargs.contains(either_str.as_cow()?.as_ref()) { | ||
match self.var_kwargs_validator { | ||
Some(ref validator) => match validator.validate(py, value.borrow_input(), state) { | ||
Ok(value) => { | ||
output_kwargs.set_item(either_str.as_py_string(py, state.cache_str()), value)?; | ||
} | ||
Err(ValError::LineErrors(line_errors)) => { | ||
for err in line_errors { | ||
errors.push(err.with_outer_location(raw_key.clone())); | ||
match self.var_kwargs_mode { | ||
VarKwargsMode::Uniform => match &self.var_kwargs_validator { | ||
Some(validator) => match validator.validate(py, value.borrow_input(), state) { | ||
Ok(value) => { | ||
output_kwargs | ||
.set_item(either_str.as_py_string(py, state.cache_str()), value)?; | ||
} | ||
Err(ValError::LineErrors(line_errors)) => { | ||
for err in line_errors { | ||
errors.push(err.with_outer_location(raw_key.clone())); | ||
} | ||
} | ||
Err(err) => return Err(err), | ||
}, | ||
None => { | ||
if let ExtraBehavior::Forbid = self.extra { | ||
errors.push(ValLineError::new_with_loc( | ||
ErrorTypeDefaults::UnexpectedKeywordArgument, | ||
value, | ||
raw_key.clone(), | ||
)); | ||
} | ||
} | ||
Err(err) => return Err(err), | ||
}, | ||
None => { | ||
if let ExtraBehavior::Forbid = self.extra { | ||
errors.push(ValLineError::new_with_loc( | ||
ErrorTypeDefaults::UnexpectedKeywordArgument, | ||
value, | ||
raw_key.clone(), | ||
)); | ||
} | ||
VarKwargsMode::UnpackedTypedDict => { | ||
// Save to the remaining kwargs, we will validate as a single dict: | ||
remaining_kwargs.set_item(either_str.as_py_string(py, state.cache_str()), value)?; | ||
} | ||
} | ||
} | ||
} | ||
|
||
if self.var_kwargs_mode == VarKwargsMode::UnpackedTypedDict { | ||
// `var_kwargs_validator` is guaranteed to be `Some`: | ||
match self | ||
.var_kwargs_validator | ||
.as_ref() | ||
.unwrap() | ||
.validate(py, remaining_kwargs.as_any(), state) | ||
{ | ||
Ok(value) => { | ||
output_kwargs.update(value.downcast_bound::<PyDict>(py).unwrap().as_mapping())?; | ||
} | ||
Err(ValError::LineErrors(line_errors)) => { | ||
errors.extend(line_errors); | ||
} | ||
Err(err) => return Err(err), | ||
} | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with DH, we decided to have everything coupled together in the
ArgumentsSchema
, as having a singleVarKwargsSchema
did not really make sense and made things harder to handle in pydantic core.This has the downside that
var_kwargs_schema
serves two purposes, depending onvar_kwargs_mode
: a core schema to validate every value of each extra kwarg, or aTypedDictSchema
to validatekwargs
entirely. Maybe we could make them two separate keys?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYM by this?
I'm alright with this as it stands now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if
var_kwargs_mode
is'single'
, thenvar_kwargs_single_schema
would have to be provided. Otherwise ifvar_kwargs_mode
is'unpacked-typed-dict
,var_kwargs_unpacked_typed_dict_schema
would have to be provided. This makes things more explicit but with more validation logic involved so probably best to keep it as is.