Skip to content

Conversation

@asnare
Copy link
Contributor

@asnare asnare commented Dec 9, 2025

Changes

What does this PR do?

This PR refactors the @lsp_feature decorator mechanism that was implemented internally to mark LSP client callbacks. We're going to have a lot more callbacks, and the existing mechanism had some drawbacks:

  • It relied on mutable global state within the module. This has been eliminated.
  • It didn't support asynchronous callbacks.

The existing mechanism has been replaced with a self-contained mechanism encapsulated within a base class. (For convenience the decorator remains a module function so it can be easily invoked.) The new mechanism also supports asynchronous callbacks.

Relevant implementation details

The PyGLS library doesn't allow for easy subclassing of LanguageClient with methods on the subclass as callbacks: callbacks can only be set up after the client has been instantiated, which poses a problem for subclassing like this. The decorator mechanism here works as such:

  • The decorator itself merely marks the callbacks, for later registration. (We use the same mechanism to do this as PyTest uses with its decorators.)
  • During initialisation registration is handled by locating marked methods on the current instance and registering them.

Tests

  • existing unit tests
  • existing integration tests

@asnare asnare self-assigned this Dec 9, 2025
@asnare asnare requested a review from a team as a code owner December 9, 2025 13:14
@asnare asnare added tech debt design flaws and other cascading effects internal technical pr's not end user facing labels Dec 9, 2025
@codecov
Copy link

codecov bot commented Dec 9, 2025

Codecov Report

❌ Patch coverage is 92.59259% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.65%. Comparing base (5a297d2) to head (f218784).

Files with missing lines Patch % Lines
...ricks/labs/lakebridge/transpiler/lsp/lsp_engine.py 92.59% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2185      +/-   ##
==========================================
+ Coverage   63.56%   63.65%   +0.09%     
==========================================
  Files         100      100              
  Lines        8503     8536      +33     
  Branches      885      887       +2     
==========================================
+ Hits         5405     5434      +29     
- Misses       2931     2934       +3     
- Partials      167      168       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

✅ 51/51 passed, 9 flaky, 4m11s total

Flaky tests:

  • 🤪 test_validate_non_empty_tables (7ms)
  • 🤪 test_validate_successful_schema_check (181ms)
  • 🤪 test_validate_mixed_checks (181ms)
  • 🤪 test_validate_invalid_schema_check (1ms)
  • 🤪 test_validate_invalid_schema_path (1ms)
  • 🤪 test_transpiles_informatica_to_sparksql_non_interactive[False] (22.273s)
  • 🤪 test_transpiles_informatica_to_sparksql (25.112s)
  • 🤪 test_transpiles_informatica_to_sparksql_non_interactive[True] (4.092s)
  • 🤪 test_transpile_teradata_sql_non_interactive[False] (5.66s)

Running from acceptance #3149

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal technical pr's not end user facing tech debt design flaws and other cascading effects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants