Update to merge algorithm for deep hashes

[karenetheridge]

This PR contains a few not-tightly-related cleanup items, and the final commit changes the way the _uniq_map merge function works: instead of borking immediately when the same hash key is found on the left and right data structures, it descends recursively into each side and tries to merge things as long as there is no conflict (duplicate values are accepted, and other non-overlapping keys are accepted). I can't think of a case where this would be the wrong thing to do.

(I also need it for pending dzil work where multiple plugins populate non-overlapping keys in meta provides.)

(Note that "final commit" means the final commit in the branch -- which is not the ordering that github shows here, because I rebased the commits to use a more sensible ordering. Thanks, github.) :p

[dagolden]

Cool! Deep merging without conflict seems quite sensible.

In the future, I'd much prefer to see "cleanup" commits clearly separated from feature change commits into a separate PR. I'm also not a big fan of some of the "remove unneeded ..." stuff from .t files, as the extra variable doesn't hurt and at least arguably makes it easier to read. I would tell you not to bother if you asked if you should do it, but since the work is done, I don't strongly object to it.

Adding the mode lines are fine (saves me from running my "\rjbs" macro) but I might also favor fixing the whitespace of the files that differ from the convention elsewhere in the dist. I don't like often like whitespace only changes on "important" projects, but for future consistency, it might be worth it. Curious what others think.

I'd like to see tests covering the cases that leont and I noted.

[karenetheridge]

I can move the other things to a separate PR. Some of them I was just going to put into master directly since they're so boring, but since I was preparing a PR I decided to keep everything together. I can move those to a separate PR, and kill the removing variables commit.

I am indifferent about reformatting the outlier files to be consistent -- since with the modelines they come out looking mostly the same, I can deal with them either way. But it was frustrating having to adjust to different indentation styles without being able to just type and not worry about it!

I'll look into the test things you both noticed when I have a minute later today.

[dagolden]

I wouldn't bother splitting it now. That's seems like make work at this point. On Apr 6, 2015 12:26 PM, "Karen Etheridge" notifications@github.com wrote:

I can move the other things to a separate PR. Some of them I was just going to put into master directly since they're so boring, but since I was preparing a PR I decided to keep everything together. I can move those to a separate PR, and kill the removing variables commit.

I am indifferent about reformatting the outlier files to be consistent -- since with the modelines they come out looking mostly the same, I can deal with them either way. But it was frustrating having to adjust to different indentation styles without being able to just type and not worry about it!

I'll look into the test things you both noticed when I have a minute later today.

— Reply to this email directly or view it on GitHub https://github.com/Perl-Toolchain-Gang/CPAN-Meta/pull/92#issuecomment-90132191 .

[karenetheridge]

I wouldn't bother splitting it now. That's seems like make work at this point.

ok :) are there any commits that can be cherry-picked now, or shall we just wait until I've addressed the code issues with merges?

[karenetheridge]

Should that be: or defined $left and defined $right and $left eq $right FWIW, I think this would be faster to grok with parens rather than having to reason through the variable precedence.

Yes, fixed, thanks! and I've added some parens.

Everything is rebased and pushed!

[dagolden]

looks good to me. I'd like to get a couple more eyes on it before merging. @rjbs?

Also, if 536f094 fails, then I'd like it squashed with the fix before the final merge happens.

[karenetheridge]

Also, if 536f094 fails...

It does not fail. All tests pass at each commit.

[Leont]

Originally I was going for "simplest feasible solution", I wasn't expecting anyone to need this kind of deep merging. It looks sensible enough to me, though I would have written it in such a way that the eval isn't necessary.

[karenetheridge]

though I would have written it in such a way that the eval isn't necessary

that's fair; I can pull out the necessary guts into an _is_identical sub.

On Sat, Apr 18, 2015 at 9:40 PM, Leon Timmermans notifications@github.com wrote:

Originally I was going for "simplest feasible solution", I wasn't expecting anyone to need this kind of deep merging. It looks sensible enough to me, though I would have written it in such a way that the eval isn't necessary.

— Reply to this email directly or view it on GitHub https://github.com/Perl-Toolchain-Gang/CPAN-Meta/pull/92#issuecomment-94195403 .

[karenetheridge]

done - previous commits are unchanged -- will squash after.

On Sat, Apr 18, 2015 at 10:19 PM, Karen Etheridge karen@froods.org wrote:

though I would have written it in such a way that the eval isn't necessary

that's fair; I can pull out the necessary guts into an _is_identical sub.

On Sat, Apr 18, 2015 at 9:40 PM, Leon Timmermans <notifications@github.com

wrote:

Originally I was going for "simplest feasible solution", I wasn't expecting anyone to need this kind of deep merging. It looks sensible enough to me, though I would have written it in such a way that the eval isn't necessary.

— Reply to this email directly or view it on GitHub https://github.com/Perl-Toolchain-Gang/CPAN-Meta/pull/92#issuecomment-94195403 .

[karenetheridge]

@dagolden approved @leont's approval and I have merged to master!