perlpunk / YAML-PP-p5

A YAML 1.2 processor in perl
https://metacpan.org/pod/YAML::PP
24 stars 8 forks source link

order is not preserved in new subhashes #44

Closed karenetheridge closed 3 years ago

karenetheridge commented 3 years ago

I wrote these test cases:

diff --git a/t/52.preserve.t b/t/52.preserve.t
index 0cfb1e2..7c7e141 100644
--- a/t/52.preserve.t
+++ b/t/52.preserve.t
@@ -124,6 +124,20 @@ EOM
     is(exists $data->{a}, 1, 'exists(a)');
     is(exists $data->{A}, '', 'exists(A)');

+    # can we preserve ordering in new subhashes?
+    $data->{d}{z} = 1;
+    $data->{d}{a} = 2;
+    @{$data->{d}}{qw(y b x c)} = 3..6;
+
+    @keys = keys %$data;
+    is("@keys", "a y b x c z d", 'keys()');
+
+    @keys = keys %{$data->{d}};
+    is("@keys", "z a y b x c", 'keys() of a new subhash');
+
+    @values = values %{$data->{d}};
+    is("@values", "1 2 3 4 5 6", 'values() of a new subhash');
+
     %$data = ();
     is(scalar keys %$data, 0, 'clear');
     is(scalar %$data, 0, 'clear');

and sadly, they fail:

...
# Subtest: preserve-order
    ok 1 - preserve=1 Key order preserved
    ok 2 - keys()
    ok 3 - hash a
    ok 4 - First key
    ok 5 - Next key
    ok 6 - delete(z)
    ok 7 - keys()
    ok 8 - keys()
    ok 9 - scalar
    ok 10 - values()
    ok 11 - exists(a)
    ok 12 - exists(A)
    ok 13 - keys()
    not ok 14 - keys() of a new subhash
    #   Failed test 'keys() of a new subhash'
    #   at t/52.preserve.t line 136.
    #          got: 'a x c b y z'
    #     expected: 'z a y b x c'
    not ok 15 - values() of a new subhash
    #   Failed test 'values() of a new subhash'
    #   at t/52.preserve.t line 139.
    #          got: '2 5 6 4 3 1'
    #     expected: '1 2 3 4 5 6'
    ok 16 - clear
    ok 17 - clear
    1..17
    # Looks like you failed 2 tests of 17.
...

I think we just need to create a new tied hash when making an assignment to a new slot.

perlpunk commented 3 years ago

Yes, that sounds like it makes sense and shouldn't be to difficult to implement. Meanwhile, you can manually create ordered hashes with YAML:PP->preserved_hash

perlpunk commented 3 years ago

OTOH what do we do in arrays that are inside a preserved hash?

perlpunk commented 3 years ago

I implemented this in https://metacpan.org/release/TINITA/YAML-PP-0.027_002 maybe you want to test it?

karenetheridge commented 3 years ago

Awesome! I will try to find some time to test it soon.

perlpunk commented 3 years ago

I actually had to add the behaviour to PUSH, SPLICE and UNSHIFT, too.

Uploaded https://metacpan.org/release/TINITA/YAML-PP-0.028

Please reopen if there are any problems :)

karenetheridge commented 3 years ago

Thanks!

Unfortunately, I found a bug. Here is a small repro case:

#!/usr/bin/env perl
use strict;
use warnings;
use YAML::PP 0.029;
use YAML::PP::Common qw(PRESERVE_ORDER PRESERVE_FLOW_STYLE);

my $yaml = YAML::PP->new(
    preserve => PRESERVE_ORDER | PRESERVE_FLOW_STYLE,
);

my $source = <<'YAML';
---
alpha: {}
YAML

my $data = $yaml->load_string($source);

$data->{alpha} = {};
$data->{alpha}{beta} = {};
$data->{alpha}{beta}{gamma}[0] = { name => 'value' };

print $yaml->dump_string($data);
__END__
got:
---
alpha:
  beta:
    gamma:
    - {}

expecting:
---
alpha:
  beta:
    gamma:
    - name: value

There is no bug if we start out with $data = {} rather than using $source from the loaded string.

perlpunk commented 3 years ago

whoops, that's right. It's because I don't actually add existing data to a given hash/array when tieing it.

I prepared a fix under: https://github.com/perlpunk/YAML-PP-p5/tree/fix-preserve

karenetheridge commented 3 years ago

Nice, that works a lot better! \o/

perlpunk commented 3 years ago

Great, thanks for testing =) I will release it in the next couple of days

karenetheridge commented 3 years ago

thanks again!