Skip to content

Refactor: expandTilde dedup, jsonrpc env+ConnectionError, _get_settings_by_path#466

Merged
edvilme merged 7 commits intomainfrom
phase4/trivial-normalizations
Apr 10, 2026
Merged

Refactor: expandTilde dedup, jsonrpc env+ConnectionError, _get_settings_by_path#466
edvilme merged 7 commits intomainfrom
phase4/trivial-normalizations

Conversation

@edvilme
Copy link
Copy Markdown
Contributor

@edvilme edvilme commented Apr 7, 2026 •
edited
Loading

Trivial normalization changes for shared package extraction. No behavioral changes.

Tracking: #465 | microsoft/vscode-python-tools-extension-template#290

  • 4.0f Deduplicate expandTilde (settings.ts -> envFile.ts)
  • 4.2b Add env parameter to start_process() in lsp_jsonrpc.py
  • 4.2c lsp_jsonrpc.py: Exception -> ConnectionError
  • 4.S2 Add _get_settings_by_path() to lsp_server.py

edvilme and others added 4 commits April 7, 2026 13:31
@edvilme
refactor: deduplicate expandTilde — single definition in envFile.ts
6546275 
Part of Phase 4 normalization (#465).
Removes duplicate expandTilde from settings.ts, exports from envFile.ts.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edvilme
refactor: add USE_DEBUGPY gate to _debug_server.py
ee74381 
Part of Phase 4 normalization (#465).
Aligns with black-formatter pattern — only loads debugpy when USE_DEBUGPY is set.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edvilme
refactor: add env parameter to start_process() in lsp_jsonrpc.py
146902d 
Part of Phase 4 normalization (#465).
Aligns with pylint pattern — allows passing environment variables to subprocess.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edvilme
refactor: align lsp_jsonrpc.py to raise ConnectionError instead of Ex…
e429794 
…ception

Part of Phase 4 normalization (#465).
Aligns with pylint pattern for more specific error typing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edvilme edvilme added the debt label Apr 7, 2026
@edvilme edvilme enabled auto-merge (squash) April 7, 2026 20:34
@edvilme edvilme changed the title Phase 4: Trivial normalizations Phase 4: expandTilde dedup, USE_DEBUGPY gate, jsonrpc env+ConnectionError, _get_settings_by_path Apr 7, 2026
@edvilme edvilme changed the title Phase 4: expandTilde dedup, USE_DEBUGPY gate, jsonrpc env+ConnectionError, _get_settings_by_path Refactor: expandTilde dedup, USE_DEBUGPY gate, jsonrpc env+ConnectionError, _get_settings_by_path Apr 7, 2026
@edvilme edvilme force-pushed the phase4/trivial-normalizations branch from ae76d78 to 83bc685 Compare April 8, 2026 20:43
rchiodo
rchiodo reviewed Apr 8, 2026
View reviewed changes
rchiodo
rchiodo reviewed Apr 8, 2026
View reviewed changes
@rchiodo
Copy link
Copy Markdown
Contributor

rchiodo commented Apr 8, 2026

There's still a bunch of unresolved comments?

@edvilme edvilme force-pushed the phase4/trivial-normalizations branch from 83bc685 to 97d40f1 Compare April 9, 2026 01:03
@edvilme
refactor: add _get_settings_by_path() to lsp_server.py
97d40f1 
Part of Phase 4 normalization (#465).
Aligns with black/isort/mypy/pylint — adds path-based settings lookup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@edvilme
Copy link
Copy Markdown
Contributor Author

edvilme commented Apr 9, 2026

Both comments addressed:

  1. Key/value mismatch: Added clarifying comment. WORKSPACE_SETTINGS is keyed by normalized workspaceFS path (see _update_workspace_settings lines 640-657), so the dict lookup is correct. The key IS the workspaceFS value.

  2. Unguarded [0] index: Added empty guard: if not setting_values: return {}

@github-actions github-actions bot mentioned this pull request Apr 9, 2026
Open
rchiodo
rchiodo reviewed Apr 9, 2026
View reviewed changes
bundled/tool/lsp_server.py
@@ -697,6 +697,23 @@ def _get_settings_by_document(document: TextDocument | None):
}

Copy link
Copy Markdown
Contributor

@rchiodo rchiodo Apr 9, 2026

Choose a reason for hiding this comment

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

Copilot generated:
-706

Prior feedback unresolved — shadow-set pattern is structurally fragile (all four reviewers agree). The comment asserting keys equal workspaceFS values doesn't prevent breakage if normalization ever diverges (trailing slash, case, slash direction on Windows). The Skeptic demonstrates a concrete scenario: if normalize_path produces backslashes but workspaceFS stores forward slashes, the in workspaces check silently fails on every ancestor and the function falls through to returning an arbitrary workspace's settings — a silent wrong-answer bug.

Replace the shadow-set + key-lookup with direct iteration:

def _get_settings_by_path(file_path: pathlib.Path):
    while file_path != file_path.parent:
        str_file_path = utils.normalize_path(file_path)
        for _key, settings in WORKSPACE_SETTINGS.items():
            if settings["workspaceFS"] == str_file_path:
                return settings
        file_path = file_path.parent

    setting_values = list(WORKSPACE_SETTINGS.values())
    if not setting_values:
        return {}
    return setting_values[0]

@rchiodo
Copy link
Copy Markdown
Contributor

rchiodo commented Apr 9, 2026

PR description claims "no behavioral changes" — Not addressed. The USE_DEBUGPY gate is a behavioral change for developers who had DEBUGPY_PATH set without USE_DEBUGPY. All four reviewers agree the description should acknowledge this.

@edvilme edvilme force-pushed the phase4/trivial-normalizations branch from 409bc23 to 4175725 Compare April 9, 2026 23:51
@edvilme edvilme force-pushed the phase4/trivial-normalizations branch from 4175725 to bee2d09 Compare April 9, 2026 23:51
@edvilme edvilme changed the title Refactor: expandTilde dedup, USE_DEBUGPY gate, jsonrpc env+ConnectionError, _get_settings_by_path Refactor: expandTilde dedup, jsonrpc env+ConnectionError, _get_settings_by_path Apr 9, 2026
@edvilme
Copy link
Copy Markdown
Contributor Author

edvilme commented Apr 10, 2026

Fixed in latest push: replaced shadow-set + key-lookup with direct iteration over WORKSPACE_SETTINGS.items(), exactly as suggested.

rchiodo
rchiodo approved these changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

Approved via Review Center.

@edvilme edvilme merged commit 01f72c2 into main Apr 10, 2026
21 checks passed
@edvilme edvilme deleted the phase4/trivial-normalizations branch April 10, 2026 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rchiodo rchiodo rchiodo approved these changes

Assignees

No one assigned

Labels

debt

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@edvilme @rchiodo