-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Add conversion to SQLite (experimental) #319
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
Conversation
7f98b59
to
e89b1f4
Compare
e89b1f4
to
7948e6e
Compare
cmd/scip/convert.go
Outdated
var indexPath, outputPath, cpuProfilePath string | ||
|
||
command := cli.Command{ | ||
Name: "convert", |
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.
convert
seems like a very generic name, especially for an experimental command.
I suggest calling it x-sqlite
– e.g. vcpkg adds x-
prefix to the commands and flags it deems experimental and not to be relied on
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.
Changed the sub-command name to be expt-convert
; users may not be familiar with the vcpkg convention on what x-
means, whereas expt
is more self-explanatory.
cmd/scip/convert_test.go
Outdated
err = converter.Convert(index) | ||
require.NoError(t, err) | ||
|
||
checkDocuments := func(db *sqlite.Conn) { |
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.
I don't think we use this type of testing very often?
Consider extracting the setup code in a separate function and turning these cases into actual tests for IDEs to pick up
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.
turning these cases into actual tests for IDEs to pick up
What do you mean by this? This command is not directly targeted for LSP/IDE use.
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.
Sorry, I meant for us, developers – TestConvert_SmokeTest
gets picked up as an individual runnable test case, but those nested functions aren't.
It's not a big deal, just not a style I've seen used in our codebases a lot/
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.
Adds a sub-command to convert a SCIP index to a SQLite database.
This is currently only meant for debugging; the schema should not be relied on.
Fixes #233
Fixes GRAPH-1200
Test plan
Added simple smoke test