Fix of #26

[mauro3]

This is an attempt to fix #26 by making BSONDict its own type. I suspect that something like this could work, but this doesn't. It fixes the behavior seen in #26 but the loading does not work yet.

Unfortunately, I don't have time to tinker more, but maybe someone else does?

[mauro3]

Cool, works now. Thanks to @Roboneet to fix the fix!

[mauro3]

Bump

[GregPlowman]

Would be good to have this fix, or is there something blocking?

[CarloLucibello]

bump

[MartinusR]

I just ran into this bug today. Is there anything I can do to get this merged?

[mauro3]

Ping @dhairyagandhi96

[DhairyaLGandhi]

Quick look seems to ask whether adding a MacroTools deep is worth the few lines of code it can save

[MartinusR]

Alternatively, from what I understoood from the code, it seems that just adding this line
lower(x::BSONDict) = BSONDict(x)
without redefining BSONDict solves the problem without breaking the tests. Though, this seems far less robust than mauro3's solution, and I may have missed some other use of lower.

EDIT: For the sake of completeness, I added a pull request with this alternative, so that you can judge by yourself.

[mauro3]

@dhairyagandhi96: where do you see a MacroTools dependency?

[GregPlowman]

Should isempty be a direct delegation?
Base.isempty(bd::BSONDict) = isempty(bd.d)

[mauro3]

yes

[DhairyaLGandhi]

All the overloads for BSONDict remind me of doing everything in one line with MacroTools.@forward

[mauro3]

Ah, now I get it, the other way around you mean. And no, I don't think it is worth it. The code as it stands is very readable.

[CarloLucibello]

@DhairyaLGandhi could you merge this or the alternative #73? Would you also consider granting some of the past contributors write access? A lot of good work has been rotting in this repo for too long

[CarloLucibello]

actually, I would merge #73 instead for the sole fact that it comes with a test

[racinmat]

If we merge this, I can make separate PR with tests so we have the fix there and tested.
cc @DhairyaLGandhi

[DhairyaLGandhi]

Be good to test as well @racinmat ! Thanks!

[CarloLucibello]

Tag a release?

[DhairyaLGandhi]

Once the tests land? ref #29 (comment)

[DhairyaLGandhi]

Seems to have caused a bug in #84, so reverting for now

[mauro3]

Oops, sorry about this. But note that I will probably have time to look into this.

[CarloLucibello]

@mauro3 if you have time to look into #89 that would be great, I have no clue of how this package works

[mauro3]

Aright, I fixed the issue in #90 (I hope).