enhancement for empty values for list and str types #5560
Conversation
WalkthroughThe changes reintroduce the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SchemaIOComponent
participant StateManager
participant CallbackHandler
User->>SchemaIOComponent: Provide raw value
SchemaIOComponent->>StateManager: set(raw value)
SchemaIOComponent->>CallbackHandler: onPathChange(raw value)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/index.tsx (1)
7-64: Consider adding TypeScript type annotations.While the changes look good, I notice this component lacks TypeScript type annotations for props, function parameters, and return values. Adding proper type definitions would improve code maintainability and provide better IDE support.
-export function SchemaIOComponent(props) { +export function SchemaIOComponent(props: { + onChange?: (state: any) => void; + onPathChange?: (path: string, value: any, schema: any, state: any) => void; + id: string; + shouldClearUseKeyStores?: boolean; + [key: string]: any; +}) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/packages/core/src/plugins/SchemaIO/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/index.tsx
🔇 Additional comments (3)
app/packages/core/src/plugins/SchemaIO/index.tsx (3)
25-25: Change that preserves original values looks good.This change correctly removes value coercion, allowing empty strings and arrays to remain as-is rather than being converted to null. This aligns with the PR objective to revert the coercion behavior added in PR #5375.
28-28: Update to onPathChange parameter matches the value handling change.This modification ensures that the raw value is consistently passed to all callbacks without coercion, maintaining consistency with the change on line 25. This change correctly supports the PR's goal of preserving empty values in their original form.
13-64:❓ Verification inconclusive
Verify impact on dependent components.
Since this change affects how values are handled (no longer coercing empty strings/arrays to null), please ensure that all components receiving values from SchemaIOComponent can properly handle empty values instead of null values.
🏁 Script executed:
#!/bin/bash # Search for components that consume SchemaIOComponent's output # Look for references to onPathChange or onChange to find consumers rg -A 10 -B 5 '<SchemaIOComponent' --type tsxLength of output: 75
Below is a revised review comment that highlights the need for manual verification due to issues with our automated search:
Verify Handling of Empty Values in Consumers
Since the change now passes empty strings/arrays rather than coercing them to null, please manually verify that all components consuming output from SchemaIOComponent correctly handle these empty values. In our attempt to automatically locate dependent components (using an rg search for '<SchemaIOComponent' in .tsx files) we encountered an error because tsx files weren’t recognized. It’s important to double-check that nothing elsewhere in the codebase relies on receiving null values for empty entries.
- Action Items:
- Manually review components that use
onChangeoronPathChangefrom SchemaIOComponent to ensure they handle empty (non-null) values correctly.- Confirm that any assumptions based on null values are updated accordingly.
There was a problem hiding this comment.
@imanjra based on your comment here, it sounds like reverting this null coercion behavior fixes your concern here. Can we find another solution to your concern on #5529 than this?
For context- the null coercion change we introduced as part of #5375 was an intentional change. The motivation was that in this common syntax:
inputs.str("str_field", required=False)the property defaults to None. However, in a dynamic form, if the user types something into str_field and then deletes it, the prop now takes value "". This is a common gotcha for plugin devs, as they don't realize that they need to handle both None and "" in their execute() method to handle the case of a user who decides they don't want to use an optional string field after all. In fact, many of the core plugins in fiftyone-plugins suffer from this bug today, which is why #5375 is so convenient; it fixed the bugs without needing to update the plugins themselves.
|
@brimoor Oh, I see. I will make some changes to support that scenario. |
imanjra
force-pushed
the
bugfix/empty-value-bug
branch
from
ee11385 to
49e4d16
Compare
imanjra
changed the title
|
Updated PR with new logic below: Only coerce value to None if it was modified by user and if the default value are not empty ("" for str and [] for list) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/packages/core/src/plugins/SchemaIO/utils/index.ts (1)
159-174: Missing TypeScript type annotations for parameters.The function implementation is correct and aligns with the PR objectives, but lacks type annotations which is inconsistent with the rest of the codebase.
Consider adding type annotations:
-export function coerceValue(path, value, schema) { +export function coerceValue(path: string, value: any, schema: SchemaType) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/packages/core/src/plugins/SchemaIO/index.tsx(2 hunks)app/packages/core/src/plugins/SchemaIO/utils/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/packages/core/src/plugins/SchemaIO/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for co...
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
app/packages/core/src/plugins/SchemaIO/utils/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
🔇 Additional comments (2)
app/packages/core/src/plugins/SchemaIO/utils/index.ts (2)
12-12: Import added to support user interaction checks.New import ensures the code can determine if a path has been modified by the user, which is essential for the new coercion logic.
133-157: New function to determine when coercion should occur.This function implements the core logic described in the PR objectives, preventing coercion of empty values to
nullwhen:
- The path hasn't been modified by the user
- The default value is an empty array (for array type)
- The default value is an empty string (for string type)
This addresses the confusion mentioned in the PR objectives where operators might expect empty strings or arrays instead of
null.
Clever! This address my comment 👍 |
There was a problem hiding this comment.
The updated behavior sounds good to me 👍
Defer to @ritch for the code review part if @imanjra would like that
What changes are proposed in this pull request?
This reverts some of the null coercion changes from #5375. Namely:
""values are NOT coerced toNoneif the default value of the field is""[]values are NOT coerced toNoneif the default value of the field is[]How is this patch tested? If it is not, please explain why.
Using various operators with list/str type
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Refactor