feat(data-fabric): align entity create field names with get response - #619
Conversation
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
1 similar comment
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
8d5f422 to
8356b10
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Review summary — 1 finding this run
|
8356b10 to
ee8bd83
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
ee8bd83 to
cc9c0f6
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
| /** Whether Analytics integration is enabled for this entity (default: false) */ | ||
| isAnalyticsEnabled?: boolean; | ||
| /** Whether Insights integration is enabled for this entity (default: false). Matches the `isInsightsEnabled` field returned by `getById`. */ | ||
| isInsightsEnabled?: boolean; |
There was a problem hiding this comment.
Please revert this. It was intentionally meant to be isAnalyticsEnabled.
There was a problem hiding this comment.
@vnaren23 We intentionally kept it as isAnalyticsEnabled But it is creating more confusion . Since Get API respond with isInsightsEnabled and create accept isAnalyticsEnabled. Instead of remapping the response of get Method , We felt if we can keep this flag same as isInsightsEnabled. Even in Python SDK we are exposing it as isInsightsEnabled only. Its is better to keep the source of truth same as API's unless necessary. Otherwise will cause more confusion if we keep it different for people using from coding agent/coded agent/coded app etc
There was a problem hiding this comment.
This is internal mapping. External contracts do not see the mapping we are doing. We intentionally took the route to keep external facing properties to be generic and not leak internal implementation details. I dont see a reason why we should revert this now.
There was a problem hiding this comment.
In that case get Response should also map it back to isAnalyticsEnabled right ? My concern is We can keep same naming convention for Create Method and Get Method.
There was a problem hiding this comment.
I'm assuming get response is already done.
cc9c0f6 to
512be20
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
512be20 to
a63a9c9
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
The entity create request used different names than the get response for the same concepts. Align the create side to the get/API vocabulary (pure renames — the flat/ergonomic create shape is unchanged): - EntityCreateFieldOptions.fieldName -> name - EntityRemoveFieldOptions.fieldName -> name - EntityCreateOptions.isAnalyticsEnabled -> isInsightsEnabled Left as-is on purpose: the flattened fields (scalar `type` + flat constraints, `referenceEntityId`/`referenceFieldId` vs nested objects) — intentional create ergonomics; and `folderKey` (SDK-wide folder input, distinct from the response's stored `folderId`). BREAKING CHANGE: EntityCreateFieldOptions.fieldName and EntityRemoveFieldOptions.fieldName are now `name`; EntityCreateOptions.isAnalyticsEnabled is now isInsightsEnabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a63a9c9 to
e25d607
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
|
3 participants
Summary
The entity create request used different names than the get response for the same concepts. This aligns the create side to the get/API vocabulary — pure renames only, the flat/ergonomic create shape is unchanged.
fieldNamenamenamefieldNamenamenameisAnalyticsEnabledisInsightsEnabledisInsightsEnabledIntentionally NOT changed
type+ flat constraints (lengthLimit,maxValue…) and bare-id references (referenceEntityId/referenceFieldId) vs the nestedfieldDataType/referenceEntity/referenceFieldin the response. These are deliberate create-input ergonomics; the SDK/backend bridge the gap (SDK mapstype⇄sqlType+fieldDisplayType; backend resolves reference ids to full objects on read).folderKeyvsfolderId—folderKeyis the SDK-wide folder-scoping input used by every service;folderIdis the entity's stored attribute in the response. Renaming would break cross-SDK consistency.Public create API renames:
EntityCreateFieldOptions.fieldName→name,EntityRemoveFieldOptions.fieldName→name,EntityCreateOptions.isAnalyticsEnabled→isInsightsEnabled.CLI follow-up needed: the
data-fabric-tooldf entities createcommand validatesfieldNameand its examples use it — it needs a pairedfieldName→nameupdate when it bumps the SDK. (isAnalyticsEnabledisn't used by the CLI create.)🤖 Generated with Claude Code