Repository navigation
Sort: a Sheet! prefix on the range sorts that sheet, not the active one - #1284
Merged
Merged
Conversation
|
Thanks for the quick fix! Sorting the named sheet, as your PR does, is
exactly the behavior we expected: an agent passing "Prices!A1:C5" means
that sheet, so please keep it that way rather than refusing the prefix. A
follow-up for the other data.rs commands (filter, remove duplicates,
subtotal) would be very welcome too, since an agent hits them the same way.
(AI-assisted testing.)
|
Contributor
Author
|
Thanks, good to know that's the behaviour you want, I'll keep it. I'll do the follow-up for filter, remove duplicates and subtotal (and the other data.rs commands that read the active sheet) in a separate PR. |
aiongg
added a commit
to aiongg/gridcraft
that referenced
this pull request
Oct 22, 2026
…Filter boxes Conflicts: - crates/engine/src/cmd/data.rs: do_sort keeps storytold#1284's sheet parameter and checks Sorting on it; toggle_filter checks Filtering after storytold#1285's data_target - crates/engine/src/tests.rs: adjacent tests, both kept
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.
Fixes #1283.
do_sortalways wrote towb.active_sheet, anddata_rangedropped theSheet!prefix ofrange, sodata.sort(andsort_range, which goes through it) sorted the same cells on the active sheet and reported success. Sort now resolves its sheet with the existingtarget_sheethelper, the wayrange.setValuesand the formatting commands do, and passes it down todo_sort;data.sortAscending/data.sortDescendingshare that path, so they get the same fix. Following the rule in #885, the selection belongs to the active sheet, so naming another sheet without arangeis an error, and so is a prefix that disagrees withsheet. Happy to switch to refusing a prefix that names another sheet instead (the #206 rule for cell references), if you'd rather keep sort on the active sheet.Thanks @Essermarth for the precise repro. Other
data.rscommands still readactive_sheetdirectly (filter, remove duplicates, subtotal, circle invalid data); I left them out to keep this one small, happy to send them as a follow-up.Validation:
tests::sort_with_a_sheet_prefix_sorts_the_named_sheet(the steps from the issue): fails on main (Pricesstaysp1, p2, p3, Sheet1 gets sorted), passes herecargo test -p gridcraft-engine --lib sort,cargo clippy -p gridcraft-engine --all-targets -- -D warnings,cargo fmt --all -- --check