chore: Merge upstream changes from main #5
Conversation
…sync-changes-from-remote-lsp-to-fork
📝 WalkthroughWalkthroughUpdates minimum Python requirement to 3.9, modernizes type hints to built-in generics, and adds a type-definition plugin with an entry point and corresponding hook. Adds signature-formatting plumbing and Formatter implementations (Black/Ruff) used by hover/completion formatting. Reworks WebSocket outbound dispatch to use an asyncio.Queue and loop.call_soon_threadsafe. Miscellaneous: logger/help string tweaks, f-string conversions, IOError→OSError changes, CI Python-matrix updates, new tests and some skips, schema/config/README/SECURITY/CHANGELOG/docs updates, funding manifest entry, and pyproject metadata/dependency adjustments. Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant PythonLSPServer
participant Plugin_type_definition
participant Jedi
Note over PythonLSPServer,Plugin_type_definition: Type-definition request flow
Client->>PythonLSPServer: textDocument/typeDefinition
PythonLSPServer->>Plugin_type_definition: pylsp_type_definition(config, document, position)
Plugin_type_definition->>Jedi: convert coords / script.infer()
Jedi-->>Plugin_type_definition: [Name objects]
Plugin_type_definition->>Plugin_type_definition: lsp_location(name) -> Location or None
Plugin_type_definition-->>PythonLSPServer: [Location dicts]
PythonLSPServer-->>Client: typeDefinition response
sequenceDiagram
autonumber
participant Client
participant WSHandler
participant PythonLSPServer
participant SendQueue
Note over WSHandler,PythonLSPServer: WebSocket outbound queue flow
Client->>WSHandler: open websocket / requests
WSHandler->>PythonLSPServer: deliver incoming messages
PythonLSPServer->>SendQueue: loop.call_soon_threadsafe(enqueue payload)
SendQueue->>WSHandler: run_server awaiting queue -> send(payload)
WSHandler-->>Client: websocket messages (responses/notifications)
sequenceDiagram
autonumber
participant Client
participant HoverCompletion
participant Config
participant Utils
participant Formatter
Note over HoverCompletion,Utils: Signature formatting path
Client->>HoverCompletion: hover or completion request
HoverCompletion->>Config: settings().get("signature", {})
Config-->>HoverCompletion: signature_config
HoverCompletion->>Utils: format_docstring(contents, markup_kind, signatures, signature_config)
Utils->>Utils: convert_signatures_to_markdown(signatures, config)
Utils->>Formatter: format_signature(signature, config, signature_formatter)
Formatter-->>Utils: formatted_signature or error
Utils-->>HoverCompletion: docstring with formatted signature (markdown)
HoverCompletion-->>Client: hover/completion response
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Update test assertions to match the new hover and signature response formats introduced in commit cc0efee. The fork now returns structured list format for hover (signature + docstring) instead of markdown dict, and plain string documentation for signatures. Changes: - Update hover tests to expect list format with separate signature code block and docstring items - Update signature tests to expect string documentation instead of dict with "value" key - Skip 5 jedi tests incompatible with Interpreter mode (extra_paths, document_path completions/definitions, file completions, references) - Add helper function for extracting hover text from list format All modified tests now pass with the custom fork modifications.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pylsp/plugins/hover.py (1)
52-52: Remove trailing whitespace.Blank line contains whitespace.
Apply this diff:
}) - + if doc:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pylsp/plugins/hover.py(1 hunks)test/plugins/test_completion.py(6 hunks)test/plugins/test_definitions.py(2 hunks)test/plugins/test_hover.py(4 hunks)test/plugins/test_jedi_rename.py(1 hunks)test/plugins/test_references.py(1 hunks)test/plugins/test_signature.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pylsp/plugins/hover.py (2)
test/fixtures.py (1)
config(133-139)pylsp/config/config.py (1)
settings(132-176)
test/plugins/test_hover.py (2)
pylsp/plugins/hover.py (1)
pylsp_hover(12-58)pylsp/workspace.py (1)
Document(396-629)
🪛 GitHub Actions: Static code analysis
pylsp/plugins/hover.py
[error] 13-13: ruff: Local variable 'signature_config' is assigned to but never used (F841). Remove assignment to unused variable.
[error] 52-52: ruff: Blank line contains whitespace (W293). Remove whitespace from blank line.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Mac Py3.9
- GitHub Check: Mac Py3.11
- GitHub Check: Mac Py3.10
- GitHub Check: Linux Py3.9
- GitHub Check: Linux Py3.10
- GitHub Check: Linux Py3.11
- GitHub Check: Win Py3.9
- GitHub Check: Win Py3.11
- GitHub Check: Win Py3.10
🔇 Additional comments (13)
test/plugins/test_references.py (1)
38-38: <!-- [scratchpad_start]
[task review_file_1/1 verify_skip_marker_1/3]
Verify indefinite skip concerns and tracking for test_references.[observations]
- Line 38: Confirmed skip with reason "Does not work with jedi.Interpreter mode (commit cc0efee)"
- test_references_builtin (line 69) remains active and unskipped
- No TODO, FIXME, or issue references in test file
- Commit "cc0efee" not found anywhere else in codebase
- No documentation or changelog entries found mentioning jedi.Interpreter or this skip
- Other skip markers in codebase use conditional skipif (IS_WIN, os.name) with platform-specific reasons
- test_references appears to test cross-file reference functionality (core LSP feature)
[analysis]
- Skip is indefinite with zero tracking references
- No issue tracker, TODO, or related documentation found
- Commit hash alone provides insufficient context
- Maintenance risk: indefinite skips tend to persist without action items
- Unlike other skips (platform-conditional), this is unconditional with no condition
[pending]
- Developer must decide: add issue tracking or remove test entirely
[actions]
Done - flag for developer verification[done]
Verified: No issue tracking exists. Confirmed indefinite skip without conditions or documentation.[scratchpad_end]
-->Verify skip or remove test.
No issue tracking found anywhere. Either add a GitHub issue link to the skip reason or remove the test if it won't be fixed.
test/plugins/test_completion.py (2)
8-8: LGTM: Type hint modernization to PEP 585.Built-in
dictreplacestyping.Dict, consistent with Python 3.9+ migration.Also applies to: 69-69
285-286: Remove verification request—test doesn't require PyQt6 installation.The test uses a string containing PyQt6 code to test Jedi's completion capability, not actual PyQt6 functionality. Like the adjacent
test_numpy_completionstest, it passes code strings topylsp_jedi_completionswithout requiring the package itself. No dependency verification needed.Likely an incorrect or invalid review comment.
test/plugins/test_signature.py (2)
65-66: LGTM.Test expectations correctly updated to match the new string-based parameter documentation format.
85-85: LGTM.Consistent update for multi-line signature test.
test/plugins/test_hover.py (6)
13-13: LGTM.Updated function signature provides better test coverage with typed parameters.
43-49: LGTM.Clean helper to extract docstrings from list-based hover results.
52-65: LGTM.Consistent use of
get_hover_texthelper for module hovers.
76-84: LGTM.Test correctly validates the new two-item hover format with signature and docstring.
88-122: Tests document that signature formatting is not applied in hover.Both tests assert identical output despite different
signatureconfig values, confirming hover returns raw Jedi signatures. This aligns with the unusedsignature_configinpylsp/plugins/hover.py.If signature formatting should be applied in hover, the implementation needs completion. Otherwise, consider removing these config-related tests or clarifying their purpose.
144-156: LGTM.Properly handles both list-based hover contents and empty results with clear fallback logic.
test/plugins/test_definitions.py (2)
6-7: LGTM: Import necessary for skip decorator.
145-145: No action needed—skip is intentional and documented.This skip is the intended fix for a known jedi.Interpreter mode limitation in the cc0efee custom fork changes. Commit 938aeb3 explicitly documents this: "Skip 5 jedi tests incompatible with Interpreter mode" with all tests now passing. The limitation is tracked by commit reference and accepted as part of the fork. There's no pending fix to track.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (4)
test/plugins/test_jedi_rename.py (1)
36-36: Use@pytest.mark.xfailwith a tracking issue reference, notskip.The prior review recommended using
@pytest.mark.xfailwith a tracking issue reference to preserve test visibility and coverage.skiphides the test from collection entirely, whereasxfaildocuments an expected failure while keeping the test in the suite. Also, replace the commit hash with a tracking issue number for clarity.Consider updating to:
-@pytest.mark.skip(reason="Does not work with jedi.Interpreter mode (commit cc0efee)") +@pytest.mark.xfail(reason="Does not work with jedi.Interpreter mode; see issue #XXXX")(Replace
#XXXXwith the actual tracking issue number.)pylsp/plugins/jedi_completion.py (2)
45-45: Use document-scoped settings for signature_config.Should be
config.settings(document_path=document.path).get("signature", {})to respect per-workspace/project settings.
187-187: Use document-scoped settings for signature_config.Should be
config.settings(document_path=document.path).get("signature", {}).pylsp/python_lsp.py (1)
449-451: Flatten typeDefinition results like definitions.Hook returns list of results; without
flattenyou'll return nested list[[...], [...]]which breaks LSP clients. Line 447 shows the correct pattern fordefinitions.Apply this diff:
def type_definition(self, doc_uri, position): - return self._hook("pylsp_type_definition", doc_uri, position=position) + return flatten(self._hook("pylsp_type_definition", doc_uri, position=position))Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pylsp/plugins/hover.py(2 hunks)pylsp/plugins/jedi_completion.py(9 hunks)pylsp/plugins/preload_imports.py(1 hunks)pylsp/python_lsp.py(13 hunks)pylsp/workspace.py(9 hunks)test/plugins/test_hover.py(4 hunks)test/plugins/test_jedi_rename.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pylsp/plugins/jedi_completion.py (4)
pylsp/config/config.py (1)
settings(132-176)pylsp/_utils.py (2)
position_to_jedi_linecolumn(349-363)format_docstring(299-334)pylsp/hookspecs.py (2)
pylsp_completion_detail(32-33)pylsp_completion_item_resolve(37-38)pylsp/plugins/rope_completion.py (2)
pylsp_completion_item_resolve(93-109)_resolve_completion(19-27)
pylsp/python_lsp.py (1)
pylsp/workspace.py (1)
get_document(111-116)
test/plugins/test_hover.py (2)
pylsp/plugins/hover.py (1)
pylsp_hover(12-59)pylsp/workspace.py (1)
Document(396-687)
pylsp/plugins/hover.py (1)
pylsp/_utils.py (1)
format_docstring(299-334)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Mac Py3.9
- GitHub Check: Mac Py3.11
- GitHub Check: Mac Py3.10
- GitHub Check: Linux Py3.9
- GitHub Check: Linux Py3.11
- GitHub Check: Linux Py3.10
- GitHub Check: Win Py3.9
- GitHub Check: Win Py3.10
- GitHub Check: Win Py3.11
🔇 Additional comments (19)
pylsp/plugins/preload_imports.py (1)
10-50: Formatting change improves maintainability.One module per line makes diffs cleaner when modules are added or removed.
pylsp/workspace.py (8)
5-5: Import modernization aligns with Python 3.9+ minimum.
importlib.metadataandcollections.abc.Generatorare appropriate for the updated baseline.Also applies to: 11-11
397-459: Consistent formatting with preload_imports.py.One-per-line style improves diff clarity.
486-486: Quote style change only.No functional impact.
491-499: Reformatted for readability.Logic unchanged; uses modern
importlib.metadataAPI.
520-520: Built-inopenis correct for Python 3.No need for
io.openwhen targeting Python 3.9+.
544-554: Good optimization: cacheself.lineslocally.Reduces lock contention and avoids repeated
splitlines()calls.
578-582: Consistent optimization pattern.Caching
self.lineslocally reduces property overhead.
645-645: Formatting change only.pylsp/python_lsp.py (9)
11-11: LGTM!Import needed for type annotation on line 511.
140-184: Verify initialization ordering to prevent race condition.
send_message(line 167) can be called viaresponse_handler(line 147) beforerun_server(line 175) initializesloopandsend_queue. Ifpylsp_handler.consume(line 163) triggers a response during the gap, line 171 will raiseAttributeError: 'NoneType' object has no attribute 'call_soon_threadsafe'.Verify that
pylsp_ws(line 143) is only invoked afterrun_serverinitializes the queue and loop, or add a guard:def send_message(message, websocket): """Handler to send responses of processed requests to respective web socket clients""" try: + if loop is None or send_queue is None: + log.warning("send_message called before initialization") + return payload = json.dumps(message, ensure_ascii=False) loop.call_soon_threadsafe(send_queue.put_nowait, (payload, websocket)) except Exception as e: log.exception("Failed to write message %s, %s", message, str(e))
260-260: LGTM!Shutdown hook call follows established pattern.
308-308: LGTM!Adds type definition provider capability.
415-415: LGTM!Type hints improve clarity; built-in generics align with Python 3.9+.
436-438: LGTM!Hook call pattern is consistent with other methods.
511-511: LGTM!Type hint uses modern built-in generics (Python 3.9+).
750-754: LGTM!Copying completion state to cell document enables proper
completionItem/resolvefor notebooks.
808-811: LGTM conditionally.Method correctly delegates to
type_definition(line 450). Ensure line 450 is fixed to flatten results.pylsp/plugins/hover.py (1)
59-59: Return shape consistency.You currently return either a list or an empty string. Consider returning MarkupContent (dict) when contentFormat=plaintext (as above) and list only for markdown to avoid mixed semantics across clients.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test/plugins/test_type_definition.py (1)
53-53: Fix typo in function name."mutli" should be "multi".
Apply this diff:
-def test_mutli_file_type_definitions(config, workspace, tmpdir) -> None: +def test_multi_file_type_definitions(config, workspace, tmpdir) -> None:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
test/plugins/test_completion.py(7 hunks)test/plugins/test_type_definition.py(1 hunks)test/test_notebook_document.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/test_notebook_document.py (3)
test/fixtures.py (3)
client_server_pair(169-179)workspace(115-120)request(57-64)test/test_utils.py (2)
send_initialize_request(64-72)send_notebook_did_open(22-31)pylsp/workspace.py (1)
documents(95-96)
test/plugins/test_type_definition.py (3)
test/fixtures.py (4)
pylsp(68-76)workspace(115-120)config(133-139)doc(143-144)pylsp/plugins/type_definition.py (1)
pylsp_type_definition(25-38)pylsp/workspace.py (1)
Document(396-687)
🪛 GitHub Actions: Static code analysis
test/plugins/test_type_definition.py
[error] 3-6: ruff check failed: Import block is unsorted or unformatted (I001). Found 1 error. 1 fixable with the --fix option.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Mac Py3.11
- GitHub Check: Mac Py3.9
- GitHub Check: Mac Py3.10
- GitHub Check: Win Py3.9
- GitHub Check: Win Py3.10
- GitHub Check: Win Py3.11
- GitHub Check: Linux Py3.9
- GitHub Check: Linux Py3.10
- GitHub Check: Linux Py3.11
🔇 Additional comments (3)
test/test_notebook_document.py (1)
492-492: Skipped tests defer coverage without a clear fix timeline.The skip decorator references commit cc0efee, which is a documented architectural change. While this explains why the tests break, it doesn't provide a path to re-enable them. Multiple tests across the codebase share this same skip reason, creating a pattern of deferred fixes. Consider:
- Adding a tracked issue or ticket to re-enable these tests post-cc0efee stabilization
- Documenting the specific jedi.Interpreter incompatibilities for future resolution
- Reviewing whether adapting the tests (as done elsewhere in the codebase) is feasible
test/plugins/test_completion.py (2)
8-8: LGTM! Clean migration to built-in generics.The removal of
Dictfrom typing imports and the update to usedict[str, TypeCase]correctly aligns with Python 3.9+ built-in generic support.Also applies to: 69-69
285-286: PyQt6 is already present in test dependencies.The pyproject.toml file includes PyQt6 in the
testoptional dependencies section. The code changes are properly supported.
Fix release workflow to properly resolve version from git tags instead
of defaulting to 0.0.0. The issue occurred because actions/checkout@v4
requires explicit tag fetching, unlike the older v2 used in upstream.
Changes:
- Add ref: ${{ github.ref }} to checkout the exact release tag
- Add fetch-tags: true to explicitly fetch all tags for setuptools_scm
This ensures setuptools_scm can properly determine the version from
git tags (e.g., v1.12.2.4-beta3 → 1.12.2.4b3) instead of falling back
to version 0.0.0.
Related: Previous releases incorrectly published as version 0.0.0
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
test/plugins/test_type_definition.py (1)
54-54: Typo in function name."mutli" should be "multi".
test/plugins/test_hover.py (2)
120-140: Identical assertions to formatting test.Opt-out test expects same output as
test_hover_signature_formatting, confirming that formatting may not be applied in the previous test.
43-50: Helper doesn't handle MarkupContent as previously suggested.The past review recommended extracting
"value"from MarkupContent dicts and from dict items within lists. Current code returns raw dicts.Apply the previously suggested fix:
def get_hover_text(result): contents = result["contents"] + if isinstance(contents, dict) and "value" in contents: # MarkupContent + return contents["value"] if isinstance(contents, list) and len(contents) > 0: - # Return the last item which is the docstring - return contents[-1] + last = contents[-1] + return last.get("value") if isinstance(last, dict) else last return contentsBased on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
test/plugins/test_hover.py(3 hunks)test/plugins/test_type_definition.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/plugins/test_type_definition.py (4)
test/fixtures.py (4)
pylsp(68-76)workspace(115-120)config(133-139)doc(143-144)pylsp/plugins/type_definition.py (1)
pylsp_type_definition(25-38)pylsp/workspace.py (1)
Document(396-687)pylsp/uris.py (1)
from_fs_path(80-85)
test/plugins/test_hover.py (3)
pylsp/plugins/hover.py (1)
pylsp_hover(12-59)test/fixtures.py (2)
doc(143-144)workspace(115-120)pylsp/workspace.py (1)
Document(396-687)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Mac Py3.11
- GitHub Check: Mac Py3.10
- GitHub Check: Mac Py3.9
- GitHub Check: Linux Py3.9
- GitHub Check: Linux Py3.10
- GitHub Check: Win Py3.11
- GitHub Check: Win Py3.9
- GitHub Check: Win Py3.10
🔇 Additional comments (9)
test/plugins/test_type_definition.py (5)
3-7: Imports correctly ordered.Third-party (pytest) precedes local imports with proper spacing.
9-23: Test data setup looks good.DOC_URI and DOC constant provide valid test fixtures.
26-39: Test logic correct.Cursor position and expected range match IntPair usage and definition.
42-50: Built-in type lookup test valid.Correctly verifies
listresolves to builtins.pyi.
53-99: Cross-file test structure sound.Despite being skipped, the test correctly sets up tmpdir modules and verifies IntPair resolution across files.
test/plugins/test_hover.py (4)
13-13: LGTM – fixture updated with parameters.
52-73: LGTM – consistent helper usage.
82-95: LGTM – handles both list and MarkupContent.Defensive dict branch future-proofs against LSP MarkupContent responses.
162-177: LGTM – graceful handling of list format and test environment limitations.Logic correctly extracts text from both dict and string items, with reasonable fallback for unresolved definitions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests