Polish Sidemantic terminal workflows#268
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e8172a19c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = super().invoke(ctx) | ||
| self._emit_deprecations(ctx) |
There was a problem hiding this comment.
Because deprecations are emitted only after super().invoke(ctx) returns, legacy aliases whose command bodies start a server (serve, api-serve, and mcp-serve call start_server, start_api_server, or mcp.run) do not show the documented deprecation warning until the server stops, which is effectively never for normal use. This makes the new lifecycle notice invisible exactly when users invoke those deprecated entry points; emit the queued warning before invoking the selected command (or from the command body) for these blocking aliases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e84de93ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result = super().invoke(ctx) | ||
| self._emit_deprecations(ctx) |
There was a problem hiding this comment.
The remaining legacy tree alias is still affected by this after-return emission path: tree is queued as deprecated, but its body calls run_workbench(...) in sidemantic/cli.py, which starts the interactive TUI and normally does not return until the user exits. The fresh evidence is that the server aliases now call emit_pending_deprecation(...), while tree still does not, so users invoking the deprecated workbench alias will not see its removal warning until after leaving the UI.
Useful? React with 👍 / 👎.
f32744e to
d1c8210
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8203827f6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return value.model_dump(mode="json", exclude_none=True) | ||
| if hasattr(value, "to_dict"): | ||
| return value.to_dict() | ||
| raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable") |
There was a problem hiding this comment.
When a query result contains common database scalar types such as Decimal, date, or datetime (for example a DuckDB SUM() over a DECIMAL amount), table/CSV rendering reaches this JSON default through _record_value and raises TypeError instead of printing the row; the new path therefore breaks default sidemantic query output for monetary/time metrics that the previous csv.writer path stringified successfully. Add conversions for these DB scalar values before falling through here.
Useful? React with 👍 / 👎.
| debug=bool(debug), | ||
| quiet=bool(quiet) if quiet is not None else bool(configured and configured.quiet), | ||
| verbose=bool(verbose) if verbose is not None else bool(configured and configured.verbose), | ||
| requested_format=output_format if output_format is not None else (configured.format if configured else None), |
There was a problem hiding this comment.
If a project sets cli.format: csv (similarly for other non-JSON defaults), this stores the config default as requested_format, so an explicit command flag like sidemantic info --json later fails with --json cannot be combined with a non-JSON --format. That violates the documented flag-over-config precedence and makes the legacy --json alias unusable until users edit their config; keep config defaults distinguishable from an explicit --format so command flags can override them.
Useful? React with 👍 / 👎.
6f73e1e to
78390a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1685a5f99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ```bash | ||
| sidemantic export-native ./adapter-project --output sidemantic.yml --validate-rust | ||
| sidemantic convert ./adapter-project --to sidemantic --output sidemantic.yml | ||
| sidemantic validate sidemantic.yml --engine rust |
There was a problem hiding this comment.
For users following this new Rust-runtime workflow, the second command validates a single sidemantic.yml file with --engine rust, but the CLI still routes Rust validation through load_graph_from_directory_with_rust(directory) and the Rust loader rejects non-directories (Path is not a directory). This means the documented replacement fails immediately after the preceding convert --output sidemantic.yml; either document validating the containing directory or update the CLI to dispatch file inputs through the Rust YAML/SQL file loaders.
Useful? React with 👍 / 👎.
e560b5e to
fb3c596
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb3c5968d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise click.UsageError( | ||
| f"{exc.message}\nDid you mean '{suggestion}'?\n" |
There was a problem hiding this comment.
Because every nested Typer app also uses this group, recording only the leaf name makes supported subcommands collide with top-level legacy aliases. In particular, sidemantic dashboard serve resolves the child name serve, queues the top-level serve deprecation, and can later warn users to use server postgres even though dashboard serve is the supported dashboard workflow; restrict this check to the root group or store/compare full command paths.
Useful? React with 👍 / 👎.
0494a0f to
af00280
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ba3daf7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for param in command.params: | ||
| if not isinstance(param, click.Argument): | ||
| continue | ||
| metavar = param.make_metavar(ctx) |
There was a problem hiding this comment.
In environments that satisfy the declared dependency range (click>=8.0.0), Click 8.0/8.1 expose make_metavar() without the ctx argument, so running the documented scripts/generate_man_page.py or calling render_manpage() raises TypeError before producing the man page. Either raise the Click minimum to the version whose API this uses or call make_metavar through a compatibility shim for older allowed Click versions.
Useful? React with 👍 / 👎.
1 participant
Adds recovery guidance, next steps, terminal documentation, TTY-aware paging, richer completion, and a documented deprecation lifecycle.