Skip to content
This repository was archived by the owner on Dec 22, 2021. It is now read-only.

Conversation

@TBSliver
Copy link
Contributor

BSON::Doc provides a _as_tied_hash function, so uses that. Also has side effect that anything with that function will work for the argument. Includes test for making sure it doesnt explode and actually works.

@coveralls
Copy link

coveralls commented Sep 14, 2018

Coverage Status

Coverage remained the same at 74.516% when pulling b1ab555 on shadow-dot-cat:TBSliver/PERL-970 into 94cb815 on mongodb:master.

Copy link
Contributor

@xdg xdg left a comment

Choose a reason for hiding this comment

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

Unless there's a particular reason to have a new .t file, I think the tests there should just wind up in t/collection.t.

elsif ( $type eq 'ARRAY' ) {
$hash->{$key} = Tie::IxHash->new( @$ref );
}
elsif ( $ref->$_can('_as_tied_hash') ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Problem with this is that even though it works, it's not an technically a Tie::IxHash. (I'm sure later code converts it.) I think you can just inline what __as_tied_hash does, ie. dereferencing the object to a list of key/value pairs to pass to the IxHash constructor. Doing that is no worse than using the private method. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea later code does convert it - putting return if $type eq 'BSON::Doc'; works as well... have added it as a logical or to the ARRAY type, as its identical conversion.

Also move tests for sorting by BSON::Doc into collection.t
@TBSliver
Copy link
Contributor Author

Have moved the tests to the end of collection.t and inlined the IxHash conversion for BSON::Doc

@TBSliver TBSliver closed this Feb 22, 2019
@TBSliver TBSliver deleted the TBSliver/PERL-970 branch February 22, 2019 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants