-
Notifications
You must be signed in to change notification settings - Fork 18
refactor!: Update FormatString expression to accept parsing context #182
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
refactor!: Update FormatString expression to accept parsing context #182
Conversation
8f6541d to
0dc7e13
Compare
To support Open Job Description modeling parsing that differs depending
on the revision and/or enabled extensions, the code needs a way to
parse differently based on those values. The chosen approach is to
include these in the parsing context.
* Introduce an abstract base class, ModelParsingContextInterface, that
every model parsing context must be a subclass of. It has variables
spec_rev and extensions, for parsing to reference.
* Modify the DynamicConstrainedStr class to require a
ModelParsingContextInterface during construction. The FormatString to
handle template the expression syntax is a subset, so modifying this
class to handle the Pydantic integration was the simplest approach.
* As part of this, change DynamicConstrainedStr and subclasses to use
__new__ instead of __init__, so they all handle construction
uniformly with the required context parameter.
* Everywhere that the tests do model validation or construct
DynamicConstrainedStr/FormatString instances, introduce the model
parsing context. In some places had to change the content of the tests
to reflect the more strict separation between model parsing and job
instantiation.
* Change various different model construction used in the tests to call
decode_job_template instead. The latter function has its own tests,
and using the specified Open Job Description syntax instead of object
constructors isolates the tests from implementation details.
* Create a function format_string_expr_parse that handles parsing a
string according to the format string expression parsing syntax, using
a model parsing context to determine the specific syntax. This does
not change the syntax, only introduces a place to add new syntax
support.
* Update the template variable reference pre-validation and template ->
job instantiation code to work with the updated DynamicConstrainedStr
that now requires a context. This required the instantiation logic to
be more clear about the separation between Template and Job, since the
latter instantiation happens without a context.
* Simplify the instantiate_model logic with a context manager to unify
handling of validation errors. This reduces code duplication, and
removed the optional loc and within_field arguments that were for
internal recursion purposes.
BREAKING CHANGE: Any creation of a DynamicConstrainedStr or FormatString
will need to provide a model parsing context, that includes the Open
Job Description revision and any extensions that are enabled. Use of
the instantiate_model can no longer provide optional loc and
within_field arguments.
Signed-off-by: Mark Wiebe <399551+mwiebe@users.noreply.github.com>
0dc7e13 to
f4f9db9
Compare
|
| for err_key, err_value in error_details.items(): | ||
| if err_key == "loc": | ||
| init_error_details["loc"] = (*loc, *err_value) # type: ignore | ||
| elif err_key != "msg": |
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.
nit, do we care about any other else? Like future "unknown" errors in case they get swallowed ?
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.
From the docs, eg "type" is also a key type.
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.
The InitErrorDetails class https://docs.pydantic.dev/latest/api/pydantic_core/#pydantic_core.InitErrorDetails is well-defined. The reason the code is filtering it out because ErrorDetails has that field but InitErrorDetails does not.


What was the problem/requirement? (What/Why)
To support Open Job Description modeling parsing that differs depending on the revision and/or enabled extensions, the code needs a way to parse differently based on those values. This is required in order to experiment with proof of concepts for OpenJobDescription/openjd-specifications#79.
The question is, where to split the processing, should we do it inside the FormatString code, or should we do it in the models that use the FormatString code. This PR chooses the former because making the code choose the behavior in FormatString will be simpler to experiment with and will decouple expression-related changes from the rest of the Open Job Description models.
What was the solution? (How)
Refactor the expression string processing to make putting together different proofs of concept for OpenJobDescription/openjd-specifications#79 easier.
What is the impact of this change?
Putting together different proofs of concept for OpenJobDescription/openjd-specifications#79 will be easier.
After this change, the code to use Python
astfor the expression parsing as considered in OpenJobDescription/openjd-specifications#79 (comment) is quite small: mwiebe@4456628.How was this change tested?
Was this change documented?
Docstrings for affected classes were updated.
Is this a breaking change?
BREAKING CHANGE: Any creation of a DynamicConstrainedStr or FormatString
will need to provide a model parsing context, that includes the Open
Job Description revision and any extensions that are enabled.
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.