Skip to content

Loader dumper objects api#563

Draft
ingydotnet wants to merge 2 commits into
release/6.0from
loader-dumper-objects-api
Draft

Loader dumper objects api#563
ingydotnet wants to merge 2 commits into
release/6.0from
loader-dumper-objects-api

Conversation

@ingydotnet
Copy link
Copy Markdown
Member

@ingydotnet ingydotnet commented Sep 23, 2021

Depends on #561

@ingydotnet ingydotnet force-pushed the loader-dumper-objects-api branch 2 times, most recently from 682fbb2 to bfc4e97 Compare September 24, 2021 20:40
Comment thread lib/yaml/constructor.py
tag_suffix = None
if node.tag in self.yaml_constructors:

if (hasattr(self, '_yaml_constructors') and
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write a wrapper for this. Not sure about a good name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to add the resolvers and representers in this PR, too?

Comment thread lib/yaml/constructor.py Outdated

if (hasattr(self, '_yaml_constructors') and
node.tag in self._yaml_constructors and
self._yaml_constructors[node.tag] is not None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be easier to delete the constructor when doing add_constructor(tag, None)? Then this check wouldn't be necessary

Comment thread lib/yaml/constructor.py

if constructor is None and hasattr(self, '_yaml_constructors'):
for tag_prefix in self._yaml_multi_constructors:
if tag_prefix is not None and node.tag.startswith(tag_prefix):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The None part of the multi_constructors below is not yet supported

Copy link
Copy Markdown
Member

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be clearer to do something like this at the start of construct_object:

if self.is_instance_based():
    constructors = self._yaml_constructors
    multi_constructors = self._yaml_multi_constructors
else:
    constructors = self.yaml_constructors
    multi_constructors = self.yaml_multi_constructors

Then later code doesn't need to be duplicated

Comment thread lib/yaml/__init__.py Outdated
# loader = Loader.get('cls')(stream)

else:
raise AssertionError("Unsupported type for Loader=")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be TypeError.

Comment thread lib/yaml/__init__.py Outdated

if loader.stream is None:
if stream is None:
raise AssertionError("load() requires stream=...")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be TypeError.

@perlpunk
Copy link
Copy Markdown
Member

One thing which is not clear from the example is an important design question:
The current PR looks into the instance based constructors (if it is an instance), and if it doesn't find the tag there, it goes on to search in the class based constructors.
Meaning, doing

l1 = yaml.SafeLoader()
yaml.add_constructor('!dice', dice_constructor, Loader=yaml.SafeLoader)

will still result in finding a constructor for !dice when using l1

My proposal would be to only lookup in one or the other - either instance or class based.
After all we want to get away from the class based global-like thing, and any new stuff would only be supported by the instance API.

@ingydotnet
Copy link
Copy Markdown
Member Author

One thing which is not clear from the example is an important design question:
The current PR looks into the instance based constructors (if it is an instance), and if it doesn't find the tag there, it goes on to search in the class based constructors.
Meaning, doing

l1 = yaml.SafeLoader()
yaml.add_constructor('!dice', dice_constructor, Loader=yaml.SafeLoader)

will still result in finding a constructor for !dice when using l1

My proposal would be to only lookup in one or the other - either instance or class based.
After all we want to get away from the class based global-like thing, and any new stuff would only be supported by the instance API.

Good point but your instance needs to at least copy over the default class constructors. iow, integers still need to work.

I still have a lot to finish up on this PR. Should have something reviewable by Monday.

Comment thread lib/yaml/__init__.py Outdated
@perlpunk
Copy link
Copy Markdown
Member

perlpunk commented Sep 25, 2021

Good point but your instance needs to at least copy over the default class constructors.

of course.

iow, integers still need to work.

integers?

@ingydotnet ingydotnet force-pushed the loader-dumper-objects-api branch from bfc4e97 to da67d4b Compare September 25, 2021 16:34
instead of just Loader* classes

All existing PyYAML usage should not be affected.
@ingydotnet ingydotnet force-pushed the loader-dumper-objects-api branch from 2495a71 to 8fb8979 Compare September 27, 2021 01:54
@ingydotnet ingydotnet self-assigned this Jan 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Won't Do

Development

Successfully merging this pull request may close these issues.

3 participants