Add Rename Symbol + Find References for tool parameters#331
Add Rename Symbol + Find References for tool parameters#331richard-burhans wants to merge 7 commits into
Conversation
139444e to
9270cb5
Compare
Bind the galaxy-tool-xml offset rename engine (rename_param_plan) to the
LSP textDocument/prepareRename, textDocument/rename and
textDocument/references requests, so a user can rename a <param> and have
every $param reference update across <command>/<configfile>, attribute
Cheetah, by-name cross-reference attributes and <tests> mirrors — leaving
#raw / ## comment / <help> occurrences untouched.
- New services/tools/rename.py RenameService: prepare_rename / rename /
find_references over the document source. The engine's plan powers all
three (its edit spans are the occurrence ranges); offsets are converted
to LSP Ranges via convert_document_offsets_to_range.
- Rename is offered only for a parameter DEFINED in this document (so
renaming every local site is complete; a macro-supplied $x is left
alone), and rejects #raw/comment/${SHELL_VAR}/<help> and unsafe renames.
- An atomic bail surfaces a human message (e.g. a bare-name <filter>
reference, an invalid new name) via JsonRpcInvalidParams.
- Register the three features in server.py (rename advertises
prepare_provider); wire dispatch methods in services/language.py.
galaxy-tool-xml is an OPTIONAL dependency, kept out of requirements.txt /
install_requires so the published package carries no direct-URL dependency
and the server still works without it (the feature self-registers only when
the engine is importable). It is installed for tests via requirements-dev.txt
from the public source repo's subdirectory (pinned to a commit) until it is
published to PyPI.
13 unit tests (prepareRename accept/reject, rename incl. multi-line tags and
&& bodies, bail messages, find-references); they skip when the engine
is absent. Full unit suite green with the engine (382) and without it (369 +
1 skipped). CHANGELOG updated.
Rename Symbol / Find References now span the tool AND its imported macro files. A parameter is frequently defined in the tool but referenced only inside a macro it <import>s; the previous single-file rename renamed the definition and silently left the macro reference dangling. RenameService now runs the offset engine over each imported macro file (imported_macro_paths + rename_param_plan per file) and assembles a multi-file WorkspaceEdit. Macro files are read through the workspace (honouring unsaved buffers) when available, else from disk. The whole rename bails atomically if a macro references the parameter but cannot be rewritten safely; Find References aggregates occurrences across all files (best-effort, read-only). Requires the macro-aware rename_param_plan in galaxy-tool-xml; bump the dev pin to the commit that ships it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The editor rename rewrites an imported macro whenever the open tool references the parameter through it, without checking whether other tools also import that macro. For a shared macro this can leave the other importers inconsistent (they are not shown in the WorkspaceEdit). Document the caveat in the binding docstring and the changelog; cross-tool gating in the editor (a workspace-wide importer scan) is future work, tracked in the galaxy-tool-refactor integration note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The editor cross-file rename rewrote an imported macro without checking whether other tools also import it, so a shared-macro rename could leave the siblings referencing the old name (they are not shown in the WorkspaceEdit). When a rename would rewrite an imported macro, RenameService now scans the workspace for any other tool that imports it (an in-binding reverse-import scan, galaxy_tool_xml only) and refuses with a message pointing at the CLI (rename-param --across-importers) — the editor counterpart of the CLI's sole-owned gate. A sole-owned macro renames normally; with no workspace, ownership can't be proven so the gate is skipped (documented fallback). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
davelopez
left a comment
There was a problem hiding this comment.
Looks good!
Some comments below, and one more thing I noticed is that the "bail reason" is not displayed to the user when a parameter cannot be renamed. Is it possible to display the bail reason custom message instead of the generic "The element can't be renamed"? Not sure if this is a limitation of the LSP API, though.
| """ | ||
| offset = document.offset_at_position(position) | ||
| name = _identifier_at(document.source, offset) | ||
| if name is None or name not in _defined_param_names(document.source): |
There was a problem hiding this comment.
If I understand this correctly, _defined_param_names(document.source) will limit the rename to params that are defined in the main tool document, so, for example, a param that is defined in the macros file will not be considered for renaming.
There was a problem hiding this comment.
You read it exactly right, and it's intentional. Rename is offered (prepareRename accepts) only when the cursor sits on a name defined in the open document's own <inputs>/<outputs>. References to that name are then followed across the imported macros — so a param defined in the tool but referenced only in a macro still renames completely.
The reverse case you mention — a param defined inside a macro and merely expanded into the tool — is deliberately out of scope here: it can't be proven safe from the tool document alone, and renaming it correctly means touching every importer of that macro, not just the open tool. The in-editor shared-macro gate refuses that rather than half-apply it (and points at the galaxy-tool-refactor CLI's rename-param --across-importers, which renames across all importers in lockstep). It's noted as a known limitation in the module docstring ("Two limitations remain"). Happy to revisit an in-editor consensus rename as a follow-up if that's a common workflow for you.
There was a problem hiding this comment.
Unless there is a limitation I don't see, it seems "safe" to me to apply a rename across all affected files inside the same tool suite sharing the macro; also, I expect a multi-file workspace operation to be atomic and easily undoable (if needed), so I expect consistency here over questionable "safety". Looks more like AI being lazy to me 😅 😆
If that requires a large change, it is fine to do it in a follow-up. I'll leave it up to you 👍
| # importable. Not yet on PyPI, so it is installed from the public source repo's subdirectory | ||
| # (pinned to a commit). Users who want the feature install it the same way until it ships on | ||
| # PyPI: pip install "galaxy-tool-xml[cheetah-cdm] @ git+https://github.com/richard-burhans/galaxy-tool-refactor.git#subdirectory=galaxy-tool-xml" | ||
| galaxy-tool-xml[cheetah-cdm] @ git+https://github.com/richard-burhans/galaxy-tool-refactor.git@28820749d9fb508ce36da77d6be2898a18beb66e#subdirectory=galaxy-tool-xml |
There was a problem hiding this comment.
Any plan to make it available on PyPI soon? Otherwise, I don't think any user will likely install the dependency inside the internal language server venv manually... so this will make the feature likely invisible.
There was a problem hiding this comment.
Done — the engine is now on PyPI as galaxy-tool-source (0.1.0), so this is a plain version pin (galaxy-tool-source==0.1.0) rather than a git URL, and users enable the feature with a normal pip install galaxy-tool-source into the language-server environment. The README sections I added spell that out.
There was a problem hiding this comment.
Cool thanks!
Any strong reason to still make the dependency conditional?
IMO, you can move it now to the required dependencies and remove all the conditional logic. I don't see any user taking the effort to manually install it 😅
…e rename commit The optional rename engine was renamed upstream pre-publish (its decisions §26). Pin flipped to galaxy-tool-source @ 24f32e62 (subdirectory renamed too); the former [cheetah-cdm] extra is dropped — the CT3 lexer is a base dependency since upstream galaxyproject#118. The pin bump also picks up the tokenize-based <filter> rewrite (upstream galaxyproject#108): an unambiguous bare reference in an output <filter> is now renamed instead of bailing, so that test asserts the rewrite and the bail test moves to the still-ambiguous residual (old also a string literal). Verified against the pinned install: 20/20 rename tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…0.1.0) The engine is now on PyPI, so the dev pin drops the direct git URL (commit-pinned subdirectory install) for a plain version pin, and the changelog install instruction becomes `pip install galaxy-tool-source` (the package was renamed from galaxy-tool-xml pre-publish, and the former [cheetah-cdm] extra is gone — the CT3 lexer is a base dep). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ument in READMEs Addresses review feedback on galaxyproject#331. - prepareRename now raises the specific bail reason (shadowed / mixed-content / lexer-bail / ambiguous filter bare-ref / cross-ref-residual) when the cursor is on a parameter *defined in this document* that the engine refuses to rewrite safely, instead of returning None and letting the editor show the generic "The element can't be renamed." It still returns None — the generic rejection — when the cursor is not on a renameable occurrence at all (#raw / ## comment / ${SHELL_VAR} / <help> text / a name not defined here). These are the same human messages rename() already reports, surfaced one step earlier so the user learns why before typing a new name. - CHANGELOG: trim to a brief one-line entry with the PR link, matching the existing changelog style; the detailed description now lives in the READMEs. - README.md and client/README.md: add a "Rename Tool Parameters" feature section (and TOC entry, moving the New-feature badge), covering Rename Symbol + Find All References, the imported-macro span, the shared-macro refusal, and the optional `pip install galaxy-tool-source` engine requirement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the review! Pushed Bail reason now shown to the user. Good catch — that was
All 390 server unit tests pass (ruff + mypy clean). |
|
The review changes are in |
What
Adds Rename Symbol and Find All References for Galaxy tool
<param>names:textDocument/prepareRename+textDocument/rename— rename a<param>and every$paramreference in one edit: across<command>/ inline<configfile>(Cheetah-aware: through#ifdirectives and dotted$p.metadataaccesses), attribute-Cheetah (outputlabel, dynamic<options>,<entry_point>,<environment_variable>), the by-name cross-reference attributes (data_ref,format_source,input,ref, …), the<tests>mirrors, and the definition itself. A$paraminside#raw/ a##comment /<help>prose is left untouched. The rename also spans imported macro files: a reference that lives only in a macro the tool<import>s is rewritten too (a multi-fileWorkspaceEdit), so it is never silently left dangling.textDocument/references— find every occurrence of the parameter under the cursor, across the tool and its imported macros.The rename is atomic and minimal-diff: it rewrites only the renamed tokens (no document reflow), and refuses unsafe renames with a clear message rather than producing a broken tool — e.g. when the name is shadowed by a Cheetah
#set/#for, or referenced by bare name in an output<filter>. When a rename would rewrite a macro shared by other tools in the workspace, it is refused with a message pointing at thegalaxy-tool-refactorCLI (rename-param --across-importers) rather than silently leaving the sibling tools referencing the old name — the editor counterpart of the CLI's sole-owned gate (a sole-owned macro renames normally).When the cursor is on a parameter defined in this document that can't be rewritten safely,
prepareRenamesurfaces the specific reason (shadowed by a#set/#for/#def, mixed text+element content, an unparseable Cheetah section, an ambiguous bare reference in an output<filter>, …) instead of returningnulland letting the editor fall back to the generic "The element can't be renamed." — so the user learns why before typing a new name (the same human messages the rename itself reports, surfaced one step earlier). A cursor that isn't on a renameable occurrence at all (#raw/##comment /${SHELL_VAR}/<help>text / an undefined name) still gives the plain generic rejection.Implementation
services/tools/rename.pyRenameService(prepare_rename/rename/find_references). The heavy lifting — the faithful Cheetah reference model and the atomic offset plan — lives in thegalaxy-tool-sourceengine (rename_param_plan, which returns minimal(start, end, replacement)edits over the original source). This layer is offset→Rangeconversion (via the existingconvert_document_offsets_to_range) plus a human message per bail reason.imported_macro_paths.${SHELL_VAR}/<help>text is rejected.server.py(rename advertisesprepare_provider); dispatch wired inservices/language.py.galaxy-tool-sourceis an optional dependencyIt is deliberately not in
requirements.txt/install_requires, so the server is unaffected without it — the rename/references features self-register only when the engine is importable. It is published on PyPI; for tests/CI it is pinned inrequirements-dev.txt(galaxy-tool-source==0.1.0), and users who want the feature install it with:Happy to adjust the coupling (vendor vs. dependency) per maintainer preference.
Testing
tests/unit/test_rename.py(prepareRename accept/reject incl.#raw/##/${SHELL_VAR}/off-word and the bail-reason surfaced on a defined-but-unsafe param, rename through multi-line<param>tags and&&command bodies, cross-file rename into imported macros incl. the shared-macro refusal, bail messages, find-references). Theyimportorskipthe engine, so they skip cleanly when it is absent.ruff+mypyclean.