Skip to content
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

Display lsp diagnostics in console #1406

Merged

Conversation

ericvergnaud
Copy link
Contributor

Our current implementation ignores LSP diagnostics.
Also it stores transpile errors in the target directory, but does not display them in the console.
This PR:

  • converts LSP diagnostics into TranspileErrors
  • displays them in the console

Fixes #1301
Supersedes #1385

@ericvergnaud ericvergnaud requested a review from a team as a code owner January 7, 2025 09:20
@ericvergnaud ericvergnaud changed the base branch from main to feature/multiplexer/refactor-transpile-error-saved January 7, 2025 09:20
@ericvergnaud ericvergnaud changed the base branch from feature/multiplexer/refactor-transpile-error-saved to feature/multiplexer/refactor-transpile-error January 7, 2025 09:21
@ericvergnaud ericvergnaud self-assigned this Jan 7, 2025
@ericvergnaud ericvergnaud added stacked PR Should be reviewed, but not merged and removed do-not-merge labels Jan 8, 2025
…ultiplexer/refactor-transpile-error

* feature/multiplexer/transpile-using-lsp:
  fix typo
  add docs
  bump max args
…ure/multiplexer/display-lsp-diagnostics-in-console

* feature/multiplexer/refactor-transpile-error:
  fix typo
  add docs
  bump max args
Base automatically changed from feature/multiplexer/refactor-transpile-error to main January 22, 2025 05:02
Copy link
Collaborator

@gueniai gueniai left a comment

Choose a reason for hiding this comment

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

lgtm

@ericvergnaud ericvergnaud removed the stacked PR Should be reviewed, but not merged label Jan 22, 2025
…in-console

* main:
  Refactor transpile error for LSP (#1405)
  Rename mock (#1423)
  Update databricks-sdk requirement from ~=0.29.0 to >=0.29,<0.42 (#1422)
  Transpile using LSP (#1404)
  Bump sqlglot from 26.0.1 to 26.1.3 (#1416)
  Configure transpile errors file (#1408)
  restore lost file and fix gitignore that led to that loss (#1414)
  bump max args (#1410)

# Conflicts:
#	src/databricks/labs/remorph/transpiler/execute.py
#	src/databricks/labs/remorph/transpiler/lsp/lsp_engine.py
#	src/databricks/labs/remorph/transpiler/sqlglot/sqlglot_engine.py
#	src/databricks/labs/remorph/transpiler/transpile_engine.py
#	tests/resources/lsp_transpiler/lsp_server.py
#	tests/unit/transpiler/test_lsp_engine.py
Copy link
Collaborator

@sundarshankar89 sundarshankar89 left a comment

Choose a reason for hiding this comment

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

LGTM


status = do_transpile(ctx.workspace_client, engine, config)
for error in errors:
print(str(error))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
print(str(error))
can we use logger instead when run in cli only stderr is redirected to console.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit

@ericvergnaud ericvergnaud added this pull request to the merge queue Jan 22, 2025
Merged via the queue into main with commit 3200080 Jan 22, 2025
5 checks passed
@ericvergnaud ericvergnaud deleted the feature/multiplexer/display-lsp-diagnostics-in-console branch January 22, 2025 09:52
ericvergnaud added a commit that referenced this pull request Jan 22, 2025
* main:
  Display lsp diagnostics in console (#1406)
  Refactor transpile error for LSP (#1405)
  Rename mock (#1423)
  Update databricks-sdk requirement from ~=0.29.0 to >=0.29,<0.42 (#1422)
  Transpile using LSP (#1404)
  Bump sqlglot from 26.0.1 to 26.1.3 (#1416)
  Configure transpile errors file (#1408)
  restore lost file and fix gitignore that led to that loss (#1414)
  bump max args (#1410)

# Conflicts:
#	README.md
#	labs.yml
#	src/databricks/labs/remorph/cli.py
#	src/databricks/labs/remorph/config.py
#	src/databricks/labs/remorph/install.py
#	src/databricks/labs/remorph/transpiler/execute.py
#	src/databricks/labs/remorph/transpiler/lsp/lsp_engine.py
#	tests/unit/test_cli_transpile.py
#	tests/unit/test_install.py
#	tests/unit/transpiler/test_execute.py
#	tests/unit/transpiler/test_lsp_engine.py
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.

[FEATURE]: Multiplexer - display transpile errors in the CLI
3 participants