Skip to content

Adds query execution capabilities to VSCode #253

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 7 commits into from
Aug 9, 2024
Merged

Conversation

ncordon
Copy link
Collaborator

@ncordon ncordon commented Jul 29, 2024

No description provided.

Copy link

changeset-bot bot commented Jul 29, 2024

⚠️ No Changeset found

Latest commit: bd03b98

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ncordon ncordon requested a review from daveajrussell July 29, 2024 17:14
Base automatically changed from adds-ndl to main July 30, 2024 08:19
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we moved this function to the commandHandlers module? thinking that most other commands registered in the registration service delegate to functions in the commandHandlers module

Copy link
Contributor

Choose a reason for hiding this comment

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

file is named resultPanel, but class is named ResultWindow - should we rename one to match the other?

Comment on lines 106 to 114

export function getErrorContent(err: Error): string {
return `
<details class="error">
<summary style="color:red">Error: ${err.message}</summary>
<pre>${err.stack}</pre>
</details>
`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see any usages of this function

Suggested change
export function getErrorContent(err: Error): string {
return `
<details class="error">
<summary style="color:red">Error: ${err.message}</summary>
<pre>${err.stack}</pre>
</details>
`;
}

@@ -47,6 +48,7 @@ type SchemaPoller = {
let _context: ExtensionContext | undefined;
let _languageClient: LanguageClient | undefined;
let _schemaPoller: SchemaPoller | undefined;
let _queryRunner: CypherRunner | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let _queryRunner: CypherRunner | undefined;
let _cypherRunner: CypherRunner | undefined;

should we call this _cypherRunner to match its type name or vice versa?

): string {
const nonce = getNonce();

return `
Copy link
Contributor

Choose a reason for hiding this comment

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

I had an idea to create a reusable webview template engine.. I've created a draft here #255

The idea is we create a common html template and stylesheets so that all webviews use the same "base", but allows for customising scripts and stylesheets

I don't know if this is premature optimisation, or if it's entirely worthwhile (wondering how many webviews we're going to create), but it tidies a few things up, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems sensible to me. What order do you want to merge them? This pr first, then the one refactoring the html template?

Copy link
Contributor

Choose a reason for hiding this comment

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

shall we merge mine later? 👍

* @return {Record<string, any>}
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export function toNativeTypes(properties: Record<string, any>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if these functions might be better served in a util-like module? they don't seem strictly related to the markup of a webview

@ncordon ncordon force-pushed the vscode-query-execution branch from 3591496 to bd03b98 Compare August 9, 2024 09:37
@@ -42,6 +42,7 @@
"postcss": "^8.4.23",
"tailwindcss": "^3.3.1",
"typescript": "^4.9.5",
"vite": "^4.2.0"
"vite": "^4.2.0",
"esbuild": "^0.19.4"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this, I struggle to build the project sometimes in osx:

@neo4j-cypher/react-codemirror-playground:build: failed to load config from /Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/vite.config.ts
@neo4j-cypher/react-codemirror-playground:build: error during build:
@neo4j-cypher/react-codemirror-playground:build: Error: The package "@esbuild/darwin-x64" could not be found, and is needed by esbuild.
@neo4j-cypher/react-codemirror-playground:build: 
@neo4j-cypher/react-codemirror-playground:build: If you are installing esbuild with npm, make sure that you don't specify the
@neo4j-cypher/react-codemirror-playground:build: "--no-optional" or "--omit=optional" flags. The "optionalDependencies" feature
@neo4j-cypher/react-codemirror-playground:build: of "package.json" is used by esbuild to install the correct binary executable
@neo4j-cypher/react-codemirror-playground:build: for your current platform.
@neo4j-cypher/react-codemirror-playground:build:     at generateBinPath (/Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/esbuild/lib/main.js:1899:15)
@neo4j-cypher/react-codemirror-playground:build:     at esbuildCommandAndArgs (/Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/esbuild/lib/main.js:1969:33)
@neo4j-cypher/react-codemirror-playground:build:     at ensureServiceIsRunning (/Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/esbuild/lib/main.js:2133:25)
@neo4j-cypher/react-codemirror-playground:build:     at build (/Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/esbuild/lib/main.js:2025:26)
@neo4j-cypher/react-codemirror-playground:build:     at bundleConfigFile (file:///Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/vite/dist/node/chunks/dep-41cf5ffd.js:66209:26)
@neo4j-cypher/react-codemirror-playground:build:     at loadConfigFromFile (file:///Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/vite/dist/node/chunks/dep-41cf5ffd.js:66185:31)
@neo4j-cypher/react-codemirror-playground:build:     at resolveConfig (file:///Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/vite/dist/node/chunks/dep-41cf5ffd.js:65782:34)
@neo4j-cypher/react-codemirror-playground:build:     at build (file:///Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/vite/dist/node/chunks/dep-41cf5ffd.js:47926:26)
@neo4j-cypher/react-codemirror-playground:build:     at CAC.<anonymous> (file:///Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemirror-playground/node_modules/vite/dist/node/cli.js:842:15)
@neo4j-cypher/react-codemirror-playground:build: npm ERR! Lifecycle script `build` failed with error: 
@neo4j-cypher/react-codemirror-playground:build: npm ERR! Error: command failed 
@neo4j-cypher/react-codemirror-playground:build: npm ERR!   in workspace: @neo4j-cypher/[email protected] 
@neo4j-cypher/react-codemirror-playground:build: npm ERR!   at location: /Users/ncordon/neo4j/intellisense/cypher-lsp/packages/react-codemir

@ncordon ncordon requested a review from daveajrussell August 9, 2024 13:48
Copy link
Contributor

@daveajrussell daveajrussell left a comment

Choose a reason for hiding this comment

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

Looks great, working as expected 👍

@ncordon ncordon merged commit aa69a2e into main Aug 9, 2024
4 checks passed
@ncordon ncordon deleted the vscode-query-execution branch August 9, 2024 14:50
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.

2 participants