Add new arguments-v3 schema
#1641
Conversation
CodSpeed Performance ReportMerging #1641 will not alter performanceComparing Summary
|
Viicos
force-pushed
the
new-arguments-schema
branch
2 times, most recently
from
2aadb68 to
8148d18
Compare
Viicos
force-pushed
the
new-arguments-schema
branch
from
4c08142 to
1555034
Compare
|
I think it makes sense to have @samuelcolvin and/or @dmontagu review here, given the relevance to |
Viicos
force-pushed
the
new-arguments-schema
branch
2 times, most recently
from
95d68a4 to
3997db9
Compare
Viicos
force-pushed
the
new-arguments-schema
branch
from
3997db9 to
b6a2ca1
Compare
| name: String, | ||
| mode: ParameterMode, | ||
| lookup_key_collection: LookupKeyCollection, |
There was a problem hiding this comment.
btw, if we are looking to reduce allocations I think we should consider seeing if we can deduplicate name and lookup_key_collection, I imagine frequently they hold the same data, copied.
Not for this PR but maybe an interesting thing to put in an issue / follow-up?
There was a problem hiding this comment.
Will create a follow-up issue
| if let Some(args) = args_kwargs.args() { | ||
| let len = args.len(); | ||
| if len > self.positional_params_count { | ||
| if let Some(var_args_param) = self.parameters.iter().find(|p| p.mode == ParameterMode::VarArgs) { |
There was a problem hiding this comment.
If we validated the structure of the parameters at build time, we might be able to just know this statically without having to iterate to find it.
There was a problem hiding this comment.
So you mean having an extra field on the validator struct, as an Option<Parameter>?
There was a problem hiding this comment.
I was thinking so, yes. Might even want something like
positional_args: Vec<Parameter>,
var_args: Option<Parameter>,
keyword_only_args: Vec<Parameter>,
var_kwargs: Option<Parameter>,There was a problem hiding this comment.
.. but also fine to leave as follow up.
| for (index, item) in args.iter().enumerate().skip(self.positional_params_count) { | ||
| errors.push(ValLineError::new_with_loc( | ||
| ErrorTypeDefaults::UnexpectedPositionalArgument, | ||
| item, | ||
| index, | ||
| )); | ||
| } |
There was a problem hiding this comment.
I think extra handling may be relevant here?
There was a problem hiding this comment.
This would align with the extra behavior for kwargs, but the current arguments schema doesn't apply anything. I'll leave it as is for now and mention in it in the follow up issue.
|
Created #1669 as a follow-up issue. |
Change Summary
Fixes pydantic/pydantic#11240
The new arguments schema allow to validate mappings (both from JSON and Python) differently (see issue), but still validates
ArgsKwargsinstances in Python mode as it does with the existing arguments schema.Trying to incorporate tests into the existing
'arguments'schema tests ended up being too complicated as the core schemas and error locs are different between core schemas.Thus, I went with completely new tests, this time with one test file per parameter kind. Existing misc.
'arguments'schema tests were mostly copied intotest_general.py/test_alias.py. Whenever possible, tests were split into multiple ones to avoid over-parametrizing, as together with Sydney we found it unclear whenexpectedcan be the successful validation result or the validation error.Related issue number
Checklist
pydantic-core(except for expected changes)