pick leftmost version-shaped token in _parse_tool_version#778
Conversation
The inline regex in _update_workspace_settings_with_version_info took a "--version" stdout, split the first line on whitespace, and kept the result only when exactly one version-shaped token appeared. When black's output also mentioned a Python version — e.g. "black, 22.3.0 (Python 3.11.4)" or "black, 26.3.1 (compiled: yes, Python 3.12.1)" — the second token would push len(parts) above 1 and the code silently fell back to "0.0.0". That literal is a valid (very old) SemVer, so the >= 22.3.0 check passed, the workspace was added to VERSION_LOOKUP, and the user saw a misleading "NOT supported" line in the log for every workspace. Pull the parsing into a tested _parse_tool_version(stdout) helper: - Scan the first line for any token shaped like X.Y or X.Y.Z followed by an optional pre-release / dev / local segment, so "3.12.0)" no longer gets returned as "3.12.0" (the trailing ")" is no longer swallowed by \S*). - When multiple candidates appear, return the leftmost — the tool name always appears before any parenthesized environment info. - Return "0.0.0" only when no candidate is found, so the existing ">=" check still produces a clear "NOT supported" line. Added bundled/tool/tests/test_parse_tool_version.py with 13 cases: standard black output, bare version, pre-release, dev with local segment, compiled and Python in parens (two forms), trailing newline, CRLF line endings, empty stdout, whitespace-only stdout, no version token, only a Python banner, and a check that the helper uses the first line only. All pass under pytest 8.4.
|
@HrachShah please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the logic that extracts Black’s version from black --version output into a dedicated _parse_tool_version(stdout) helper and adds focused unit tests to prevent regressions across common real-world stdout formats.
Changes:
- Added
lsp_server._parse_tool_version(stdout)to pick the leftmost “version-shaped” token from the first stdout line, returning"0.0.0"only when no candidate exists. - Replaced the prior inline version-parsing logic in
_update_workspace_settings_with_version_infowith the new helper. - Added a new pytest module with coverage for standard, pre-release/dev/local, parenthetical Python info, CRLF, empty/whitespace output, and multi-line stdout cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| bundled/tool/lsp_server.py | Introduces _parse_tool_version and wires it into workspace version detection; also includes newly added module-level tool configuration/constants (currently problematic). |
| bundled/tool/tests/test_parse_tool_version.py | Adds unit tests covering many black --version stdout variants to validate version parsing behavior. |
| TOOL_MODULE = "black" | ||
| TOOL_DISPLAY = "Black Formatter" | ||
|
|
||
| # Default arguments always passed to black. | ||
| TOOL_ARGS = [] | ||
|
|
||
| # Minimum version of black supported. | ||
| MIN_VERSION = "22.3.0" | ||
|
|
||
| BLACK_CONFIG = ToolServerConfig( | ||
| tool_module=TOOL_MODULE, | ||
| tool_display=TOOL_DISPLAY, | ||
| tool_args=TOOL_ARGS, | ||
| min_version=MIN_VERSION, | ||
| runner_script=str(RUNNER), | ||
| ) | ||
|
|
||
| tool_server = ToolServer(BLACK_CONFIG, server=LSP_SERVER) | ||
|
|
||
| WORKSPACE_SETTINGS = tool_server.workspace_settings | ||
| GLOBAL_SETTINGS = tool_server.global_settings |
| # Minimum version of black that supports the `--line-ranges` CLI option. | ||
| LINE_RANGES_MIN_VERSION = (23, 11, 0) | ||
|
|
||
| # Timeout in seconds for formatting operations to prevent indefinite blocking. | ||
| FORMATTING_TIMEOUT = 120 | ||
|
|
||
| # Versions of black found by workspace | ||
| VERSION_LOOKUP: Dict[str, Tuple[int, int, int]] = {} | ||
|
|
| return "0.0.0" | ||
| # When multiple version-shaped tokens are present, prefer the leftmost so | ||
| # the real tool version beats any "Python 3.12" fragment in parentheses. | ||
| return candidates[0] |
There was a problem hiding this comment.
The "leftmost version-shaped token wins" heuristic only fails safe for black by coincidence: a leading Python 3.x banner is always < 22.3.0 (black's CalVer), so it correctly lands on "NOT supported". Since this file is the shared vscode-python-tools template and the helper is generically named _parse_tool_version, copying it to a tool whose real version is in the 2.x/3.x range (e.g. isort/autopep8) would mistake a leading Python banner for a supported version. Consider anchoring selection on TOOL_MODULE, or at least add a comment noting the leftmost rule is only safe here because black's version numbers exceed CPython's.
|
|
||
| import lsp_server # noqa: E402 | ||
|
|
||
|
|
There was a problem hiding this comment.
bundled/ is the shipped VSIX payload. Please confirm .vscodeignore excludes bundled/**/tests/** (or relocate these tests to the conventional src/test/python_tests/ location) so test code isn't shipped in the extension.
|
|
||
| def test_dev_version_with_local_segment(self): | ||
| # PEP 440 dev / local-version: 24.3.0.dev1+g1234 | ||
| assert ( |
There was a problem hiding this comment.
The PR headline is that a trailing ) (e.g. 3.12.0)) is no longer swallowed, but in every test the returned leftmost token is followed by whitespace, so the )-stripping only ever affects the non-returned second candidate. Add a case that pins the headline behavior on the returned token, e.g. assert _parse_tool_version("black, 22.3.0)") == "22.3.0" (and ideally the no-space "black,22.3.0)" form).
|
The core change is solid and nicely tested. A few small, non-blocking suggestions below (pin the trailing- |
Pull the parsing into a tested _parse_tool_version(stdout) helper:
Added bundled/tool/tests/test_parse_tool_version.py with 13 cases: standard black output, bare version, pre-release, dev with local segment, compiled and Python in parens (two forms), trailing newline, CRLF line endings, empty stdout, whitespace-only stdout, no version token, only a Python banner, and a check that the helper uses the first line only.