pyyaml icon indicating copy to clipboard operation
pyyaml copied to clipboard

Added style checks on emitter method. Related to #401

Open athossampayo opened this issue 2 years ago • 3 comments

As said on related issue #401 This bug only happens on PyYAML and not on ruamel.yaml. On ruamel.yaml they are checking for the existence of event.style attribute, and on PyYAML we are not doing this check:

ruamel.yaml:

def check_simple_key(self):
        (...)
        return length < self.MAX_SIMPLE_KEY_LENGTH and (
            isinstance(self.event, AliasEvent)
            or (isinstance(self.event, SequenceStartEvent) and self.event.flow_style is True)
            or (isinstance(self.event, MappingStartEvent) and self.event.flow_style is True)
            or (
                isinstance(self.event, ScalarEvent)
                # if there is an explicit style for an empty string, it is a simple key
                and not (self.analysis.empty and self.style and self.style not in '\'"')
                and not self.analysis.multiline
            )
            or self.check_empty_sequence()
            or self.check_empty_mapping()
        )

pyaml:

def check_simple_key(self):
        (...)
        return (length < 128 and (isinstance(self.event, AliasEvent)
            or (isinstance(self.event, ScalarEvent)
                    and not self.analysis.empty and not self.analysis.multiline)
            or self.check_empty_sequence() or self.check_empty_mapping()))

So I added the said checks.

I tried to add a test for this case but did not find an easy way, so if anyone can help on it, will be great!

athossampayo avatar Nov 14 '21 19:11 athossampayo

I tried to study how the test suite works, and this is my best guess on how to make a minimal test case for empty string keys:

tests/data/empty-string-key.code:

{
    "": "foo",
}

tests/data/empty-string-key.data:

---
'' : foo

But why doesn't python tests/lib/test_build.py fail if I drop these files on master?

It does fail if I e.g. change foo to bar in only one of those two files.

akaihola avatar Mar 28 '22 19:03 akaihola

I had the same results as @akaihola adding the tests on master first. I guess it has something to do with the "Skipped 4 dump tests" that is printed while tests are running?

athossampayo avatar Mar 28 '22 20:03 athossampayo

I hope a maintainer picks this up and finds out the correct way to write the test.

akaihola avatar Apr 23 '22 14:04 akaihola