Conventional dataset tag#359
Conversation
WalkthroughAdds Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider returning a more semantically appropriate status code (e.g., 204 No Content) from the new
POST /datasets/{identifier}/tagsendpoint instead of200with anullbody, since no response payload is provided. - The deprecated
tag_datasethandler now calls the newtag_dataset_newroute function directly; it would be cleaner to extract the shared tagging logic into a separate helper/service function that both endpoints call to avoid coupling between router handlers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider returning a more semantically appropriate status code (e.g., 204 No Content) from the new `POST /datasets/{identifier}/tags` endpoint instead of `200` with a `null` body, since no response payload is provided.
- The deprecated `tag_dataset` handler now calls the new `tag_dataset_new` route function directly; it would be cleaner to extract the shared tagging logic into a separate helper/service function that both endpoints call to avoid coupling between router handlers.
## Individual Comments
### Comment 1
<location path="docs/migration.md" line_range="136" />
<code_context>
+???+ warning "This endpoint is deprecated"
+
+ The new tag endpoint is `POST` `/datasets/{identifier}/tags`.
+ The dataset identifier is provided as part of the path, the tag is still provided in the body.
+ This endpoint is provided for easier migration in case the response matters to you.
+
</code_context>
<issue_to_address>
**issue (typo):** Fix the comma splice between the two independent clauses.
You can fix this by replacing the comma with "and", using a semicolon, or splitting it into two sentences.
```suggestion
The dataset identifier is provided as part of the path and the tag is still provided in the body.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ???+ warning "This endpoint is deprecated" | ||
|
|
||
| The new tag endpoint is `POST` `/datasets/{identifier}/tags`. | ||
| The dataset identifier is provided as part of the path, the tag is still provided in the body. |
There was a problem hiding this comment.
issue (typo): Fix the comma splice between the two independent clauses.
You can fix this by replacing the comma with "and", using a semicolon, or splitting it into two sentences.
| The dataset identifier is provided as part of the path, the tag is still provided in the body. | |
| The dataset identifier is provided as part of the path and the tag is still provided in the body. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
- Coverage 94.65% 94.64% -0.01%
==========================================
Files 77 77
Lines 3815 3827 +12
Branches 248 248
==========================================
+ Hits 3611 3622 +11
Misses 137 137
- Partials 67 68 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/migration.md (1)
135-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the replacement request body explicitly.
The new endpoint requires an embedded payload such as
{"tag": "foo"}; showing this example would prevent clients from carrying over the legacydata_idfield during migration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/migration.md` around lines 135 - 137, Update the migration documentation describing the POST /datasets/{identifier}/tags endpoint to explicitly show the replacement request body, using a payload example with the tag field such as {"tag": "foo"} and omitting the legacy data_id field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/migration.md`:
- Around line 135-137: Update the migration documentation describing the POST
/datasets/{identifier}/tags endpoint to explicitly show the replacement request
body, using a payload example with the tag field such as {"tag": "foo"} and
omitting the legacy data_id field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 412be693-b754-4414-adab-161cbc1fdad6
📒 Files selected for processing (4)
docs/migration.mddocs/roadmap.mdsrc/routers/datasets.pytests/routers/dataset_tag_test.py
Add a
datasets/{identifiers}/tagsendpoint