Conversation
Fresh-eyes review on #1843 (2026-09-17) surfaced three pre-existing bugs that the samples/_shared.py migration in that PR did not introduce but did make more visible: 1. Line 127 used a bare type annotation `changed: TSC.CustomViewItem( id=c.id, name=...)` instead of an assignment, so `changed` was never bound and the following `server.custom_views.update(changed)` would NameError. 2. `c` was defined inside the custom-views loop and then referenced outside it. On a site with zero custom views the loop never runs and every subsequent reference NameErrors. 3. `if args.delete:` at line 145 referred to a flag that was never defined in the sample's argparse, so any invocation reaching that line raised AttributeError. Fixes: - Add `=` on line 127 to bind `changed`. - Collect the custom-views iterator into a list, wrap the update/ export block in `if custom_views:`, and pick the last entry explicitly (`c = custom_views[-1]`) — matches the intent of the original "for the last custom view in the list" comment. - Add a `--delete` argparse flag with `action="store_true"` and a clear help string. Guard the delete block so it prints a clear no-op message when the site has no custom views. No other behavior change. Sample compiles and imports cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The --delete path can still fail when the site has no workbooks.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes three runtime bugs in the workbook exploration sample’s custom-view workflow.
Changes:
- Adds the
--deleteflag. - Corrects custom-view update assignment and selection.
- Handles empty custom-view lists.
File summaries
| File | Summary |
|---|---|
samples/explore_workbook.py |
Repairs custom-view update, export, and deletion logic; a remaining empty-workbook edge case requires correction. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
153
to
+155
| if args.delete: | ||
| print(f"deleting {c.id}") | ||
| unlucky = TSC.CustomViewItem(c.id) | ||
| server.custom_views.delete(unlucky.id) | ||
| if not custom_views: | ||
| print("--delete requested but no custom views on this site; nothing to delete.") |
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.
Motivation
Fresh-eyes review on #1843 (2026-09-17) surfaced three pre-existing
bugs in
samples/explore_workbook.py. They pre-date #1843 (that PRonly swapped the inline argparse for
_shared.add_common_arguments),but running the sample as documented has been broken for a while.
Splitting them out here rather than expanding the samples PR's scope.
Bugs
Line 127 — bare type annotation,
changednever bound.changed: TSC.CustomViewItem(id=c.id, name="I was updated by tsc")used
:instead of=, sochangedwas never assigned and thenext line's
server.custom_views.update(changed)raisedNameError: name 'changed' is not defined.creferenced outside its defining loop.cwas the loop variable offor c in TSC.Pager(server.custom_views):.Every subsequent reference (
c.id,c.image, delete block) ranoutside the loop. On a site with zero custom views the loop never
executes and
cis unbound — the whole custom-view sectionNameErrors on empty sites.
if args.delete:references an undefined argparse flag.The sample checked
args.deleteat line 145 but never defined--deletein argparse, so any invocation reaching that branchraised
AttributeError: 'Namespace' object has no attribute 'delete'.Fixes
=on line 127 to bindchanged.if custom_views:, and pick the last entry explicitly(
c = custom_views[-1]) — matches the intent of the original"for the last custom view in the list" comment.
--deleteargparse flag (action="store_true") with a clearhelp string. Guard the delete block so it prints a no-op message
when the site has no custom views instead of NameErroring.
No other behavior change.
Test plan
python -c "import ast; ast.parse(open('samples/explore_workbook.py').read())"— parses.--deleteflag deletes it.🤖 Generated with Claude Code