Skip to content

Improve error message when removing a workshop twice - #990

Draft
jonathan-conder wants to merge 1 commit into
mainfrom
fix/workshop-already-removed-message
Draft

Improve error message when removing a workshop twice#990
jonathan-conder wants to merge 1 commit into
mainfrom
fix/workshop-already-removed-message

Conversation

@jonathan-conder

@jonathan-conder jonathan-conder commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Description

This is a minor API break: v1PostProjectWorkshop returns 404 when a workshop isn't found. TBH not sure if this is a good idea.

Before:

$ workshop remove dev
error: cannot remove "dev": workshop not launched

After:

$ workshop remove dev
error: cannot remove "dev": workshop already removed

The intent of not launched is to hint that the user should launch the workshop; in this case that doesn't apply.

Self-review quick check

  • Make decisions that cost a lot to reverse explicit in the PR description.
  • Avoid nested conditions.
  • Delete dead code and redundant comments.
  • Normalise symmetries by sticking to doing identical things identically.
// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...
  • Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
  • Put variable declaration and initialisation together.
  • Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
  • Put a blank line between two logically different chunks of code.
  • Follow the style guide for new error messages.

Docs

Procedure:

  • I have checked and added or updated relevant documentation.
  • I have checked and added or updated relevant release notes.
  • I have included the technical author in the review.

Content:

  • Headings and titles accurately describe the content.
  • New and updated pages include correct metadata.
  • Documentation tests are added or updated where applicable (for tutorial/ and how-to/ sections).
  • Documentation follows the style guide.
  • If needed, docs/.coverage.yaml updated, coverage tags added (.. artefact).

Or:

  • I confirm the PR has no implications for documentation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Improves removal errors when the requested workshop is already absent.

Changes:

  • Wraps ErrWorkshopNotLaunched with a clearer removal-specific message.
  • Updates API test expectations for the new message.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/overlord/workshopstate/manifest.go Adds the removal-specific wrapped error.
internal/daemon/api_workshops_test.go Updates expected API errors.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jonathan-conder
jonathan-conder requested a review from akcano August 17, 2026 23:03
@jonathan-conder
jonathan-conder force-pushed the fix/workshop-already-removed-message branch from 46127d6 to 2e7704d Compare August 17, 2026 23:43
@jonathan-conder
jonathan-conder force-pushed the fix/workshop-already-removed-message branch from 2e7704d to 1ee939d Compare August 18, 2026 00:40
@vikorama

vikorama commented Aug 18, 2026

Copy link
Copy Markdown

What would it say when the workshop has never been launched (hence never been removed)?

Cross-posting my MM proposal here:

I'd suggest to output a non-error message
"nimble" status is Off; nothing to remove.

When a user tries to remove several workshops, it'll be
"nimble" status is Off; nothing to remove
"jazzy" removed

The current error should stay as is when the status is pending.

@jonathan-conder
jonathan-conder marked this pull request as draft August 18, 2026 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants