Skip to content

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

Merged
merged 3 commits into from
May 9, 2025
Merged

Conversation

varungandhi-src
Copy link
Contributor

@varungandhi-src varungandhi-src commented Apr 29, 2025

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

var indexPath, outputPath, cpuProfilePath string

command := cli.Command{
Name: "convert",

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

Copy link
Contributor Author

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.

err = converter.Convert(index)
require.NoError(t, err)

checkDocuments := func(db *sqlite.Conn) {

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

Copy link
Contributor Author

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.

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/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Broke up the logic into individual functions and sub-tests within the same test; GoLand will let you run sub-tests individually.

CleanShot 2025-05-09 at 15 27 33@2x

@varungandhi-src varungandhi-src merged commit 0c0c2b8 into main May 9, 2025
3 checks passed
@varungandhi-src varungandhi-src deleted the vg/sqlite-2 branch May 9, 2025 10:32
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.

SCIP sqlite database?
2 participants