feat(datasets): narrow create to sql/query-id; rename label/table-name#95
Open
eddietejeda wants to merge 7 commits into
Open
feat(datasets): narrow create to sql/query-id; rename label/table-name#95eddietejeda wants to merge 7 commits into
eddietejeda wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review
Blocking Issues
src/datasets.rs:386—create_from_queryerrors out witherror: --label is required when using --sqlwhendescriptionisNone. With the new CLI,--descriptionis documented as optional, so the documented happy path (hotdata datasets create --sql "..." --name foo) crashes, and the message references a flag that no longer exists.src/datasets.rs:403— same issue increate_from_saved_queryfor the--query-idpath.src/datasets.rs:510—datasets::updatestill printserror: provide at least one of --label or --table-name., both of which were renamed in this PR.
Action Required
- Decide how
create_from_query/create_from_saved_queryshould handle a missing--description(fall back toname, pass through to the API, or make--descriptionrequired) and update the error messages to reference the new flag names. - Update the
datasets::updateno-fields error message to reference--description/--name.
Non-blocking nits left inline.
After the create rework removed --file/--upload-id/--url paths, the upload infrastructure (FileType, detect_from_bytes/path, stdin_redirect_filename, make_progress_bar, do_upload, upload_from_file/stdin, create_from_upload, create_from_url) was left as dead code. Remove it. Also align create_from_query/create_from_saved_query with the new CLI: - `name: &str` (required, was Option<&str>) - `description: Option<&str>` (optional, was required label) - create_dataset builds the body with table_name always set, label only when provided Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Review
Blocking Issues
src/datasets.rs:218—datasets::updatestill printserror: provide at least one of --label or --table-name., referencing the old flag names. This was flagged in the prior review and remains unfixed. The two other prior blocking issues (create_from_query/create_from_saved_queryrejectingNonefor label) are resolved.
Action Required
- Update the
datasets::updateno-fields error message to reference--descriptionand--name.
Contributor
There was a problem hiding this comment.
Review
Blocking Issues
src/datasets.rs:218—datasets::updatestill printserror: provide at least one of --label or --table-name., referencing the old flag names. Flagged in cycles 1 and 2; still unresolved in this cycle.
Action Required
- Update the
datasets::updateno-fields error message to reference--descriptionand--name.
4 tasks
- datasets::update now references --description/--name (not the old --label/--table-name flags that were renamed in this PR) - replace query_id.unwrap() with unreachable!() to make the clap invariant explicit rather than silently panicking Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@claude review |
Without this, omitting --description sent no label to the API, which treats label as required and would return a server-side error. Also corrects the --name help text example from a qualified default.public.my_view to just my_view — the API table_name field expects an unqualified identifier. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All prior blocking issues are addressed: create_from_query and create_from_saved_query no longer reject None for description; datasets::update error message at src/datasets.rs:216 now references the new --description / --name flags; query_id is unwrapped via unreachable! to make the clap invariant explicit. LGTM.
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
1 participant
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.
Summary
datasets createnow only accepts--sqlor--query-idas the data source —--file,--upload-id,--urlremoved from the CLI (for file uploads, usehotdata databases)--sqlor--query-idis required at parse time--name(required) replaces--table-name— the SQL table name the dataset is registered under--description(optional) replaces--label— the human-readable display namedatasets updatefor consistencydatasets.rsare kept but no longer exposed