-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
Remove legacy JSON code #1356
Remove legacy JSON code #1356
Conversation
Adjust for namespace `vast::format::json` and modify json import integration tests to be based on simdjson.
This is a backup commit which stores a compilable version. Writer test fails and some other tests are temporary disabled.
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
f59dbb4
to
626dc89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good as far as I can tell.
edit: I did test the output of vast version
and vast status
. The other stuff should be covered by CI.
They are both covered by integration tests. The example plugin integration test checks |
@@ -13,6 +13,20 @@ This changelog documents all notable user-facing changes of VAST. | |||
|
|||
## Unreleased | |||
|
|||
### ⚡️ Breaking Changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, was this added intentionally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, that was definitely a mistake. Can you remove this in your other PR that's likely next to be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one for the partition_selector is probably next, but that doesn't touch the changelog. I'll remove it in the PR that moves around the options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
📔 Description
This PR fixes up @ngrodzitski's work in #1343 so we can merge it into master.
📝 Checklist
🎯 Review Instructions
Commit-by-commit.