Skip to content

Add PlanChanges and DeclarativeToImperative to API#667

Merged
morgo merged 7 commits into
block:mainfrom
morgo:add-declarative-to-imperative
Mar 18, 2026
Merged

Add PlanChanges and DeclarativeToImperative to API#667
morgo merged 7 commits into
block:mainfrom
morgo:add-declarative-to-imperative

Conversation

@morgo
Copy link
Copy Markdown
Collaborator

@morgo morgo commented Mar 18, 2026

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.
Further notes in https://github.com/block/spirit/blob/main/.github/CONTRIBUTING.md

This adds lint.PlanChanges() and statement.DeclarativeToImperative(). They are related:

  • PlanChanges() calls DeclarativeToImperative() first and then adds in lint suggestions
  • You can call either, but I think most of our usages will call PlanChanges() to get the lint suggestions.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new declarative schema-diff API in statement and a higher-level planning API in lint that combines diffing with lint suggestions, then refactors the diff command to use these shared primitives.

Changes:

  • Add statement.DeclarativeToImperative() to compute CREATE/ALTER/DROP statements from current vs desired schemas.
  • Add lint.PlanChanges() to return semicolon-terminated DDL plus per-change lint results (warnings/errors/infos).
  • Refactor pkg/lint diff command internals to use DeclarativeToImperative() and add new tests for both packages.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
pkg/statement/declarative.go New exported declarative diff function and CreateTable.ToTableSchema() helper.
pkg/statement/declarative_test.go New tests for declarative diff behavior and ToTableSchema().
pkg/lint/declarative.go New PlanChanges() API and Plan/PlannedChange types for diff + lint output.
pkg/lint/declarative_test.go New tests for PlanChanges() behavior, including lint infos/warnings.
pkg/lint/cmd_diff.go Refactor diff computation to call statement.DeclarativeToImperative().
pkg/lint/cmd_diff_test.go Remove test tied to the old CREATE TABLE restore helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/statement/declarative.go
Comment thread pkg/statement/declarative.go
Comment thread pkg/statement/declarative.go Outdated
Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/statement/declarative_test.go Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a new declarative schema-diff API in statement and a higher-level lint.PlanChanges() helper that plans schema changes and annotates them with lint results. It also refactors the lint diff command/tests to use the new planning API.

Changes:

  • Add statement.DeclarativeToImperative() (and CreateTable.ToTableSchema()) to compute DDL changes from current vs desired schemas.
  • Add lint.PlanChanges() to produce semicolon-terminated DDL plus per-statement lint warnings/errors/infos.
  • Update lint diff command and tests to use PlanChanges instead of bespoke diff logic.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pkg/statement/declarative.go New declarative-to-imperative diff function + ToTableSchema() helper.
pkg/statement/declarative_test.go New unit tests for DeclarativeToImperative() and ToTableSchema().
pkg/lint/declarative.go New Plan / PlannedChange types and PlanChanges() implementation.
pkg/lint/declarative_test.go New tests covering PlanChanges() behavior (including info severity + multi-statement tables).
pkg/lint/cmd_diff.go Refactor diff command to use PlanChanges() for declarative diffs.
pkg/lint/cmd_diff_test.go Update diff command tests to validate the new plan-based output behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/lint/cmd_diff.go
Comment thread pkg/lint/cmd_diff.go Outdated
Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/lint/declarative.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a declarative diff API (statement.DeclarativeToImperative) and a higher-level planning API (lint.PlanChanges) that combines schema diffing with linting, and updates the diff command/tests to use the new planning flow.

Changes:

  • Add statement.DeclarativeToImperative() to compute imperative DDL (CREATE/ALTER/DROP) from current vs desired table schemas.
  • Add lint.PlanChanges() to produce per-statement DDL plus attached lint violations (errors/warnings/infos).
  • Refactor lint diff command/tests to use PlanChanges (and remove the old bespoke diff logic).

Reviewed changes

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

Show a summary per file
File Description
pkg/statement/declarative.go New exported declarative diff function and CreateTable.ToTableSchema() helper.
pkg/statement/declarative_test.go Tests for DeclarativeToImperative and ToTableSchema.
pkg/lint/declarative.go New PlanChanges API and Plan/PlannedChange structures.
pkg/lint/declarative_test.go Tests covering PlanChanges behavior and lint attachment.
pkg/lint/cmd_diff.go Switch declarative diff path to PlanChanges + SQL output via printPlan.
pkg/lint/cmd_diff_test.go Update diff command tests to assert against PlanChanges output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/lint/declarative.go Outdated
Comment thread pkg/lint/declarative_test.go
Copy link
Copy Markdown
Collaborator

@aparajon aparajon left a comment

Choose a reason for hiding this comment

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

Nice API — PlanChanges combining diff + lint into a single call with per-statement violations is a big ergonomic win for downstream consumers. A few notes inline, mostly around safety for callers who don't control their input schemas.

— Claude Code

Comment thread pkg/statement/declarative.go
Comment thread pkg/lint/declarative.go
// - desired: the desired table schemas (CREATE TABLE DDL)
// - diffOpts: options for the diff (nil uses defaults)
// - lintConfig: configuration for linting (nil uses defaults; LintOnlyChanges is always overridden to true)
func PlanChanges(current, desired []table.TableSchema, diffOpts *statement.DiffOptions, lintConfig *Config) (*Plan, error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should PlanChanges accept []table.TableSchema for current schemas, or would it also make sense to accept []*statement.CreateTable directly? Downstream consumers that already have parsed CreateTable objects (e.g., from table.LoadSchemaFromDB) would avoid a round-trip through ToTableSchema()ParseCreateTable() here.

Not blocking — the TableSchema input is cleaner as a public API contract. Just wondering if a convenience overload would help.

— Claude Code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No change right now, reasonable suggestion, but the other callers prefer []table.TableSchema.

Comment thread pkg/lint/declarative.go
Comment thread pkg/statement/declarative.go
@morgo morgo enabled auto-merge March 18, 2026 14:06
@morgo morgo merged commit b19c85c into block:main Mar 18, 2026
10 checks passed
@morgo morgo deleted the add-declarative-to-imperative branch March 18, 2026 14:09
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