Promote "filter" as the preferred name for the "update" tool group#235
Open
cpsievert wants to merge 15 commits into
Open
Promote "filter" as the preferred name for the "update" tool group#235cpsievert wants to merge 15 commits into
cpsievert wants to merge 15 commits into
Conversation
The `tools` parameter now defaults to `c("filter", "query")` /
`("filter", "query")` instead of `c("update", "query")`. The legacy
name `"update"` is still accepted and silently normalized to `"update"`
internally, so no existing code breaks — only the user-facing default
and documentation change.
Closes #222
- Deduplicate after filter→update normalization so c("filter", "update")
doesn't produce duplicate tools (R: unique(), Python: dict.fromkeys)
- Add tests for the "filter" alias in both R and Python
- Add changelog entries to NEWS.md and CHANGELOG.md
set() doesn't guarantee iteration order, which broke tests on Python 3.14
where ("update", "query") became ("query", "update").
set() doesn't guarantee iteration order, so tests should compare as sets rather than asserting exact tuple order.
…l-to-filter # Conflicts: # pkg-r/DESCRIPTION # pkg-r/man/QueryChat.Rd
…ield normalize_tools() used set() for deduplication which destroyed the user-specified ordering. Switch to dict.fromkeys() which deduplicates while preserving insertion order. Remove duplicate Config/roxygen2/version in pkg-r/DESCRIPTION that caused pak dependency resolution to fail.
The test was waiting for a `.shiny-tool-result` element that only appears when the LLM calls a tool. When the LLM answers with text instead of a tool call, the test times out after 90s. Wait for the assistant's response text instead — the core assertion (no fullscreen toggle on non-viz results) still holds regardless of whether a tool was invoked.
This test depended on the LLM calling the query tool for a specific prompt, which isn't guaranteed. The fullscreen toggle is only added by the visualize tool's ToolResultDisplay(full_screen=True), so non-viz results inherently can't have it.
…string Address Copilot PR feedback: - normalize_tools now returns set[str] instead of tuple, matching the actual usage (only membership checks, no ordering dependency) - Clarify tools="filter" docstring to note it also omits "visualize" - Remove unused TOOL_GROUPS imports from internal modules - Update all test assertions to compare sets directly
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #222
Users think of this tool as "filtering" their data, but the
toolsparameter has always called it"update". This promotes"filter"as the user-facing name while keeping full backward compatibility.What changed
toolsparameter now defaults to("filter", "query")instead of("update", "query")across both R and Python packages (all frameworks: Shiny, Streamlit, Dash, Gradio)"filter"is silently normalized to"update"internally, so all existing internal code paths, LLM tool names, annotation titles, and callback parameter names remain untouched"update"is still accepted everywhere — existing user code doesn't break"filter"and note"update"as a legacy aliasset[str]in Python (instead oftuple) since only membership checks are used — no code depends on orderingtest_non_viz_tool_results_have_no_fullscreenplaywright test — it was timing-sensitive and unrelated to this rename