Skip to content

Conversation

@gonfunko
Copy link
Contributor

The basics

The details

Resolves

Fixes #8167

Proposed Changes

This PR updates Workspace.undo() to have a default argument of false, corresponding to undo, and adds a tiny Workspace.redo() wrapper method. This allows users to simply .undo() or .redo() without having to remember or correctly reason about the argument, and preserves backwards compatibility.

@gonfunko gonfunko requested a review from a team as a code owner January 15, 2026 17:40
@gonfunko gonfunko requested a review from BenHenning January 15, 2026 17:40
@github-actions github-actions bot added the PR: fix Fixes a bug label Jan 15, 2026
Copy link
Collaborator

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @gonfunko! Another nice simplification. Just had one suggestion on improving testing but otherwise this LGTM.

/**
* Redoes the previous action.
*/
redo() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small suggestion: it would be nice to have a few new tests specifically for this method (since it's now part of the class's public API which means it has its own contract that should be enforced). Presumably these could just be a copy of the redo undo tests, but I also don't think they should replace those since undo's contract isn't changing (other than the new defaulting behavior and I don't think that needs to necessarily be tested).

@BenHenning BenHenning assigned gonfunko and unassigned BenHenning Jan 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: fix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve readability and maintainability of undo method

3 participants