extract _parse_tool_version helper for black --version parsing#762
extract _parse_tool_version helper for black --version parsing#762HrachShah wants to merge 2 commits into
Conversation
… helper _update_workspace_settings_with_version_info inlined a fragile regex match against the first line of the tool's --version output: \d+\.\d+\S*. If the line was empty (splitlines() raises IndexError) the whole initialization crashed; if the first line had no version token, or had multiple (e.g. a CPython banner like 'Python 3.12.0'), the code defaulted to the literal string '0.0.0' which is a valid (very old) SemVer and silently passes the >= 22.3.0 check while logging a misleading 'NOT supported' line for every workspace. Pull the regex match out into _parse_tool_version(stdout) and call it from the version detection loop with proper ValueError handling: empty stdout -> 'empty --version output'; no candidate -> 'no version candidate in --version output: <line>'. When the first line has multiple version-shaped tokens, pick the longest so a real '24.3.0' or '24.3.0rc1' wins over a stray short match in a Python banner. The previous code only kept a match when len(parts) == 1, silently dropping valid versions whenever the line had any other version-shaped text. Added bundled/tool/tests/test_parse_tool_version.py with 9 unit tests: standard black output 'black, 24.3.0', pre-release '24.3.0rc1', multiple candidates, CRLF line endings, empty stdout, whitespace-only first line, missing version token, two-part version '24.3', and confirms the helper uses the first line only.
|
Danke shon! @HrachShah |
| def test_handles_two_part_version(self): | ||
| out = "black, 24.3\n" | ||
| assert _parse_tool_version(out) == "24.3" | ||
|
|
There was a problem hiding this comment.
📍 bundled/tool/tests/test_parse_tool_version.py:52
Confirmed violation of tests-no-partial-asserts.md: test_raises_on_whitespace_only_output and test_raises_when_no_version_token use match="no version candidate", a partial regex, against fully deterministic exception messages. The inputs are hardcoded constants, so the full message is fixed and must be asserted exactly: match=r"no version candidate in --version output: '\s*'" and match=r"no version candidate in --version output: 'something went wrong'". (test_raises_on_empty_output already uses a full match and is fine.)
| # If the first line contains multiple version-shaped tokens (e.g. black | ||
| # with an embedded 'X.Y.Z' and '24.3.0'), pick the longest so a | ||
| # pre-release suffix like '24.3.0rc1' wins over a stray short match. | ||
| return max(parts, key=len) |
There was a problem hiding this comment.
📍 bundled/tool/lsp_server.py:348
max(parts, key=len) picks the wrong token on a same-length tie: _parse_tool_version("Python 3.12.0 black, 24.3.0") returns "3.12.0", not 24.3.0, because max returns the first maximum. The PR's multi-token test only passes because rc1 makes black's token coincidentally longer — the test is tuned to the heuristic, not the real format. Reachability is low (black emits a single version token on line 1), so this is not a blocker, but consider anchoring on the format instead — e.g. the first token that fullmatches after black, — and retargeting the test.
| # A second line that contains a version is ignored; we only look | ||
| # at the first line because that's where tools emit their banner. | ||
| out = "black, 24.3.0\nsome other 1.2.3.4 token\n" | ||
| assert _parse_tool_version(out) == "24.3.0" |
There was a problem hiding this comment.
📍 bundled/tool/tests/test_parse_tool_version.py:1
The tests exercise _parse_tool_version in isolation only. The PR's most consequential behavioral change — the new continue skipping VERSION_LOOKUP population and the resulting potential KeyError in range formatting — has no integration coverage. A unit test of the helper can never catch this. Consider adding a test that drives the version-detection loop with unparseable --version output and then asserts range formatting still degrades gracefully rather than raising.
| # If the first line contains multiple version-shaped tokens (e.g. black | ||
| # with an embedded 'X.Y.Z' and '24.3.0'), pick the longest so a | ||
| # pre-release suffix like '24.3.0rc1' wins over a stray short match. | ||
| return max(parts, key=len) |
There was a problem hiding this comment.
📍 bundled/tool/lsp_server.py:332
The PR description overstates/misstates the motivation: the old splitlines()[0] IndexError was caught by the surrounding bare except: so initialization did not crash (one workspace was skipped); (0,0,0) >= (22,3,0) is False so the old default logged "NOT supported" rather than silently passing the check; and the old \d+\.\d+\S* regex already matched a bare 24.3, so the (?:\.\d+)? addition doesn't change that case. The refactor still stands on its own merits — please correct the description so future readers trust the rationale. Info-only.
|
Refactor looks good and is safe to merge. A few non-blocking notes below: a tie-break edge case in |
|
Solid refactor with good unit coverage. One non-blocking note on the version-token selection heuristic; the remaining first-pass items were either test-style nitpicks, an inapplicable internal review rule, or PR-description nits and were dropped. |
|
Thanks for your contribution @HrachShah ! |
|
@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”),
|
_update_workspace_settings_with_version_info inlined a fragile regex match against the first line of the tool's --version output. If the line was empty, splitlines() raised IndexError and the whole initialization crashed. If the first line had no version token, or had multiple (e.g. a CPython banner like 'Python 3.12.0'), the code defaulted to the literal string '0.0.0' which is a valid (very old) SemVer and silently passes the >= 22.3.0 check while logging a misleading 'NOT supported' line for every workspace.
This commit pulls the regex match out into _parse_tool_version(stdout) and calls it from the version detection loop with proper ValueError handling: empty stdout -> 'empty --version output'; no candidate -> 'no version candidate in --version output: '. When the first line has multiple version-shaped tokens, the helper picks the longest so a real '24.3.0' or '24.3.0rc1' wins over a stray short match in a Python banner.
Also tightens the regex from \d+.\d+\S* to \d+.\d+(?:.\d+)?\S* fullmatch, so a bare '24.3' is accepted (some dev installs drop the patch number) without matching '24.3.0' twice on a '24.3.0rc1' line.
Added bundled/tool/tests/test_parse_tool_version.py with 9 unit tests covering: standard 'black, 24.3.0' output, pre-release '24.3.0rc1', multiple version tokens on the first line, CRLF line endings, empty stdout, whitespace-only first line, missing version token, two-part version '24.3', and confirms the helper uses the first line only.