Skip to content

Add support for local dbs in config#1751

Merged
shati-patel merged 5 commits intomainfrom
shati-patel/local-db-config
Nov 14, 2022
Merged

Add support for local dbs in config#1751
shati-patel merged 5 commits intomainfrom
shati-patel/local-db-config

Conversation

@shati-patel
Copy link
Copy Markdown
Contributor

@shati-patel shati-patel commented Nov 11, 2022

Adds support for local databases the config for the new databases panel. Note: this only changes the config - we don't yet render the local items (that's a follow-up issue).

See internal issue for context!

Checklist

N/A - internal only 🖌️

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Looking good so far, just some small comments!

name: string;
dateAdded: number;
language: string;
path: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about storagePath instead?

local: {
lists: config.local.lists.map((list) => ({
name: list.name,
databases: list.databases.map((db) => ({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this would work too:

databases: list.databases.map((db) => ({ ...db }));

"name": "foo/bar",
"dateAdded": 1668096745193,
"language": "go",
"path": "Uri to db in workspace storage"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess it could just be the file system path rather than a URI? What do we store currently?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We currently store a Uri:

readonly databaseUri: vscode.Uri;

We could change to a file system path, but I wonder if that would make it harder to integrate with existing stuff?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm Uri is a VS Code thing so I'm thinking we should stick to filesystem paths. We can easily translate to vscode.Uri by doing vscode.Uri.file('/path/to/dir').

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. Will change that 👍🏽

Comment thread extensions/ql-vscode/workspace-databases-schema.json
@shati-patel shati-patel force-pushed the shati-patel/local-db-config branch from 7eccfb8 to 9db9e7e Compare November 11, 2022 16:18
@shati-patel
Copy link
Copy Markdown
Contributor Author

I'm going to add some more local cases to the tests, and then I'll take this out of draft :)

@shati-patel shati-patel marked this pull request as ready for review November 14, 2022 09:52
@shati-patel shati-patel requested review from a team as code owners November 14, 2022 09:52
Comment thread extensions/ql-vscode/src/databases/db-config.ts Outdated
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM!

@shati-patel shati-patel merged commit 3fd9fd4 into main Nov 14, 2022
@shati-patel shati-patel deleted the shati-patel/local-db-config branch November 14, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants