Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add BSON marshalling #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

morgasaurus
Copy link

@morgasaurus morgasaurus commented Apr 22, 2018

Includes GetBSON and SetBSON methods and a unit test comparing String outputs of decimals before and after being marshalled and unmarshalled via struct and map.

Uses string parsing to convert to and from the bson.Decimal128 structure used in MongoDb 3.4.

Edit: It looks like the CI build failed since it is not finding the BSON package dependency.

@victorquinn
Copy link
Contributor

Hrm, change looks great! But I'm hesitant to merge it without the test suite running properly.

I'd also prefer not to put hurdles in place for future contributors (like having to know to run go get gopkg.in/mgo.v2/bson in order to run the tests).

I think we should figure out dependency management here unfortunately, or find another way to test it without adding an external dependency. Previously this module had no dependencies so there isn't any dependency management here so unfortunately that'll be a bit of a beast. That said, I don't see a great way to move forward here without the dependency as trying to write tests without it would be somewhat of a pain.

Any thoughts @morgasaurus ?

@peterstace
Copy link

The new dependency is also introduced due to bson.Raw in the SetBSON method, so it's not just for testing. That's a shame, since users of decimal who have no interest in BSON support will have to add the bson lib as a transitive dependency to their projects.

@morgasaurus
Copy link
Author

morgasaurus commented Apr 24, 2018

Unfortunately as peter said there isn't any way around the dependency because the BSON Setter interface has bson.Raw in method signature.

The advantage is support for MongoDb Decimal128 via BSON, and the disadvantage is introduction of the BSON dependency. A third option is to have another branch with the BSON support.

I want to thank y'all because the decimal repository has been extremely useful and saved a lot of errors that were propagating from float64. If you decide not to merge, then I can certainly maintain the fork for projects. I just thought I would offer the pull request since it looked like there was an open issue and we ran into a similar one.

If you do decide to merge, then in the future we may look at improving the BSON Get/Set by digging into bits instead of string parsing which should perform better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants