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

bson: support all non-deprecated types and fix int/uint bugs #650

Merged
merged 3 commits into from
May 2, 2023

Conversation

matthewdale
Copy link
Contributor

  • Add support for reading javascript elements.
  • Add support for reading decimal128 elements (only printing as binary for now, with a TODO to parse the decimal128 value).
  • Add support for reading minkey and maxkey elements
  • Fix BSON document length: read int32 instead of uint32.
  • Fix binary element size: read int32 instead of uint32.
  • Fix datetime: read int64 instead of int32.
  • Update BSON test file to include at least one field for every supported type.

@wader
Copy link
Owner

wader commented Apr 30, 2023

👍 Nice, will review later this evening

doc/formats.md Show resolved Hide resolved
@@ -1,7 +1,6 @@
package bson

// https://bsonspec.org/spec.html
// TODO: more types
Copy link
Owner

Choose a reason for hiding this comment

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

🥳

format/bson/bson.go Show resolved Hide resolved
format/bson/bson.go Show resolved Hide resolved
# Run the code at https://go.dev/play/p/8YuESN04ACr and copy the base64 output. Then run:
#
# echo "<base64 output>" | base64 -D > test.bson
#
Copy link
Owner

Choose a reason for hiding this comment

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

I think it makes sense to add the generate program to a file under format/bson/testdata. Some reason it has to go thru base64 btw?

Copy link
Owner

Choose a reason for hiding this comment

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

Also thanks for good tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I can add the source for the Go program used to generate the test BSON document in format/bson/testdata. There's no reason it needs to use base64, it was just a reliable way to copy binary data from a web browser into a terminal.

Copy link
Owner

Choose a reason for hiding this comment

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

Aha makes sense

@wader
Copy link
Owner

wader commented Apr 30, 2023

Looks good 👍 just some minor things about docs and tests

@matthewdale matthewdale requested a review from wader May 1, 2023 03:53
@wader
Copy link
Owner

wader commented May 1, 2023

Will merge once CI is green. Thanks a lot!

@wader
Copy link
Owner

wader commented May 1, 2023

Fails on testing fq -h bson, help(bson)/help("bson") output:

--- FAIL: TestFormats (0.40s)
    --- FAIL: TestFormats/bson/testdata/help_bson.fqtest (0.03s)
        fqtest.go:15: 
            --- expected
            +++ actual
            @@ -8,6 +8,10 @@
               $ fq -d bson . file
               # Decode value as bson
               ... | bson
            +
            +Limitations:
            +
            +- The decimal1[28](https://github.com/wader/fq/actions/runs/4848437587/jobs/8641291596?pr=650#step:5:29) type is not supported for decoding, will just be treated as binary
             
             Convert represented value to JSON
             =================================
FAIL

Sorry I forgot in the review that bson.md should use markdown 3-level sections so change Limitations: to ### Limitations.
After that run WRITE_ACTUAL=1 go test -run TestFormats/bson ./format and will rewrite the test output.

To be clear the markdown in bson.md is used both for formats.md and in the builtin help system where it's rendered a bit differently to be CLI friendly. Have some vague plan of using them to generate man pages etc also.

BTW if you want you can also add authors section, something like:

### Authors
- Mattias Wadman mattias.wadman@gmail.com, original author
- Matt Dale <contact info if you like>, additional types and bug fixes 

@matthewdale
Copy link
Contributor Author

@wader Thanks for the feedback and recommendations! I think I've made all the changes and fixes you suggested.

@wader wader merged commit ddd7b0e into wader:master May 2, 2023
5 checks passed
@wader
Copy link
Owner

wader commented May 2, 2023

@matthewdale 🥳 Thanks and let me know if you have any question about the other improvements we talking about, happy to help out!

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.

None yet

2 participants