-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[dagster-yaml] yaml pydantic parsing utils #21824
Conversation
914722a
to
a83d2dc
Compare
af61cdb
to
eec9bf4
Compare
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.
This is slick - few pedantic comments inline but impl looks good to me
obj._source_position_and_key_path is not None # noqa: SLF001 | ||
and obj._source_position_and_key_path != source_position_and_key_path # noqa: SLF001 | ||
): | ||
raise RuntimeError( |
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.
What's the reason for this? Just to make sure we don't accidentally do duplicate work?
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.
Yeah just something has probably gone wrong if we end up here. Changed it to a check.invariant
.
source_position = SourcePosition( | ||
filename=filename, | ||
start=LineCol( | ||
line=node.start_mark.line + 1, |
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.
assume this is zero-index-to-one-index? might be worth noting inline or in LineCol
docstring that that is the case
class SafeLineLoader(yaml.SafeLoader): | ||
@classmethod | ||
def remove_implicit_resolver(cls, tag_to_remove: Any) -> None: | ||
if "yaml_implicit_resolvers" not in cls.__dict__: |
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.
what's the deal with this? assume it's some yaml
pkg weirdness, but is there some weird logic where yaml_implicit_resolvers
is available via attribute access but is not actually an attribute on the class?
this fn feels all like a little bit of black magic since it's dealing w/ SafeLoader
internals, could be good to add some context
cb39a4e
to
e2fb58c
Compare
branch-name: dagster-yaml-utils
## Summary & Motivation Tools for parsing YAML files into pydantic models, preserving information about source positions, including those source positions in validation errors. Adapted from @petehunt's versions of these in the internal repo. ## How I Tested These Changes
Summary & Motivation
Tools for parsing YAML files into pydantic models, preserving information about source positions, including those source positions in validation errors.
Adapted from @petehunt's versions of these in the internal repo.
How I Tested These Changes