-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #4890 #4891
Fix #4890 #4891
Conversation
WalkthroughThe pull request introduces new constants and test cases in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/packages/looker/src/elements/common/bubbles.test.ts (2)
200-215
: Add assertion to verify the result of getBubbles for classifications.The new test case for classifications is well-structured and consistent with existing tests. However, it's missing an assertion to verify the result of the
getBubbles
call. Consider adding an expectation to ensure the function behaves correctly with the classifications input.Here's a suggested improvement:
expect( getBubbles( "classes", { classes: { classifications: [{ label: "label" }] } }, { classes: classifications } ) ).toEqual([classifications, [{ classifications: [{ label: "label" }] }]]);
217-231
: Add assertion to verify the result of getBubbles for temporal detections.The new test case for temporal detections is well-structured and consistent with existing tests. However, like the classifications test, it's missing an assertion to verify the result of the
getBubbles
call. Consider adding an expectation to ensure the function behaves correctly with the temporal detections input.Here's a suggested improvement:
expect( getBubbles( "temporal", { temporal: { detections: [{ label: "label" }] } }, { temporal: temporalDetections } ) ).toEqual([temporalDetections, [{ detections: [{ label: "label" }] }]]);app/packages/looker/src/elements/common/bubbles.ts (1)
57-60
: Enhance type safety by avoiding unnecessary type assertionsThe use of type assertions (
as Sample[]
) could potentially mask type mismatches and suppress compiler warnings. Consider refining the types returned by theunwind
function and theflatMap
callbacks to ensure TypeScript can infer the correct types without assertions. This improves type safety and helps catch potential issues at compile time.Also applies to: 64-67
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/packages/looker/src/elements/common/bubbles.test.ts (2 hunks)
- app/packages/looker/src/elements/common/bubbles.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
app/packages/looker/src/elements/common/bubbles.test.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/elements/common/bubbles.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (2)
app/packages/looker/src/elements/common/bubbles.test.ts (2)
3-8
: LGTM: New imports added correctly.The new constants
CLASSIFICATIONS
andTEMPORAL_DETECTIONS
are imported consistently with the existing import style. These imports are necessary for the new test cases added later in the file.
Line range hint
1-331
: Overall, good additions to test coverage, but assertions need improvement.The changes to this file enhance the test suite by adding new cases for classifications and temporal detections. These additions are valuable for ensuring the
getBubbles
function handles these new data types correctly. The structure and style of the new tests are consistent with existing ones, which is commendable.However, both new test cases are missing assertions to verify the results of the
getBubbles
calls. Adding these assertions, as suggested in the previous comments, will significantly improve the effectiveness of these tests and ensure that the function behaves as expected for these new data types.These changes align well with the PR objective of addressing issues related to
Classifications
andTemporalDetections
in the FiftyOne application. By improving test coverage for these components, we can better ensure their proper functionality and prevent future regressions.To ensure comprehensive test coverage, let's verify if there are any other files that might need similar test updates:
This script will help identify other files that might need similar test updates or implementations related to Classifications and TemporalDetections.
✅ Verification successful
Verification Complete: No Additional Test Files Require Updates.
The executed scripts confirmed that there are no other test files within
app/packages/looker/src/
that mentionClassifications
orTemporalDetections
. This means that the current test coverage for these components is comprehensive within the scope of the reviewed files.The initial review comment about enhancing the assertions in
bubbles.test.ts
remains pertinent to ensure robust validation of thegetBubbles
function.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find other test files that might need updates for Classifications and TemporalDetections # Search for test files that mention Classifications or TemporalDetections echo "Test files that might need updates:" rg -l --type ts "Classifications|TemporalDetections" "app/packages/looker/src/**/*.test.ts" # Search for non-test files that use Classifications or TemporalDetections echo "\nNon-test files using Classifications or TemporalDetections:" rg -l --type ts "Classifications|TemporalDetections" "app/packages/looker/src/**/*.ts" | grep -v ".test.ts"Length of output: 564
Script:
#!/bin/bash # Description: Find other test files that might need updates for Classifications and TemporalDetections # Search for test files that mention Classifications or TemporalDetections echo "Test files that might need updates:" fd --type f --extension ts --glob '*.test.ts' | xargs rg --files-with-matches "Classifications|TemporalDetections" # Search for non-test files that use Classifications or TemporalDetections echo "\nNon-test files using Classifications or TemporalDetections:" fd --type f --extension ts --exclude '*.test.ts' | xargs rg --files-with-matches "Classifications|TemporalDetections"Length of output: 790
@@ -54,14 +54,14 @@ export const getBubbles = ( | |||
} | |||
|
|||
if (field.embeddedDocType === withPath(LABELS_PATH, CLASSIFICATIONS)) { | |||
out.values = out.values.flatMap( | |||
out.values = unwind(field.dbField, out.values).flatMap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does out.values look like? Flat map should unnest arrays so it's weird that you would need to unwind first
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out.values is a list or single value of potentially renderable field data. flatMap flattens the nested classifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
What changes are proposed in this pull request?
Resolves #4890
How is this patch tested? If it is not, please explain why.
Unit tests for the
Classifications
andTemporalDetections
special casesRelease Notes
What areas of FiftyOne does this PR affect?
fiftyone
Python library changesSummary by CodeRabbit
New Features
Bug Fixes
getBubbles
function to correctly process nested data structures.Tests
getBubbles
function with new scenarios for classifications and temporal detections.