Looker typing cleanup #5536
Looker typing cleanup #5536
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes refactor the internals of the Looker package to improve encapsulation and clarity. In the Changes
Sequence Diagram(s)sequenceDiagram
participant AL as AbstractLooker
participant AM as AsyncLabelsRenderingManager
participant W as Worker
AL->>AM: enqueueLabelPaintingJob({ sample, options })
AM->>W: assignJobToFreeWorker(job)
W-->>AM: WorkerResponse<S>
AM-->>AL: AsyncJobResolutionResult<S>
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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/looker/src/worker/index.ts (1)
318-327: Consider optional or default fields
Declaring all properties as required inProcessSampleOptionsmight be too strict if some properties can be missing or have default behaviors. If that’s the case, consider marking them optional or providing defaults.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/packages/looker/src/lookers/abstract.ts(4 hunks)app/packages/looker/src/worker/async-labels-rendering-manager.ts(5 hunks)app/packages/looker/src/worker/index.ts(4 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/looker/src/worker/index.tsapp/packages/looker/src/worker/async-labels-rendering-manager.tsapp/packages/looker/src/lookers/abstract.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (21)
app/packages/looker/src/worker/index.ts (5)
11-15: Import usage looks consistent
The newly introduced imports (getCls,getFetchFunction, andsetFetchFunction) appear properly utilized and align with the rest of the module.
328-332: Check intersection type correctness
Sample & FrameSampleassumes allSampleinstances always have frame attributes. Verify that no usage scenario lacks frame properties; otherwise, a more flexible type might be needed.
338-347: Destructuring approach is clear
The function signature effectively groupsoptionsfields. This improves readability and maintainability.
365-365: Initialize buffers array
DefiningmaskTargetsBuffershere clarifies that the gathered buffers are meant for subsequent transfer.
863-877: Consolidated worker arguments
Packagingoptions,sample, anduuidintoworkerArgsoffers clarity when passing data to the worker. The approach is consistent with the rest of the system.app/packages/looker/src/worker/async-labels-rendering-manager.ts (8)
5-5: Use of Schema
ImportingSchemahere integrates well with the typed approach in the rest of the file.
7-8: New imports for process-sample types
The imported types are used consistently for strongly-typed job submission and handling.
14-21: AsyncLabelsRenderingJob type enhancements
Specifying properties likeoptionsandsamplewithin the job definition clarifies the data flow for label-rendering tasks.
23-27: AsyncJobResolutionResult
Defining a clear resolution result type promotes maintainable asynchronous patterns.
28-33: WorkerResponse type
Including bothsampleandcoloringin the worker’s response clarifies the data returned to the main thread.
39-39: Use of pendingJobs map
Storing jobs keyed by sample is a straightforward approach to avoid redundant label-processing tasks.
165-165: Generic AsyncLabelsRenderingManager
Allowing a generic sample type broadens the manager’s compatibility with different sample structures.
171-197: enqueueLabelPaintingJob modifications
Leveraging the new consolidatedoptionsstructure and generics improves the API’s clarity, reduces parameter sprawl, and assists in type safety.app/packages/looker/src/lookers/abstract.ts (8)
85-88: Refactoring core properties into private fields
This grants stricter encapsulation, ensuring these fields are only accessible internally.
92-92: Privatizing eventTarget
RestrictingeventTargetto class-internal usage reduces unintended external manipulation of events.
96-96: Private resizeObserver
Localizing the resize logic within the class prevents misuse or accidental external control.
100-100: Introduce isSampleUpdating flag
This boolean effectively prevents concurrent sample loading, improving reliability.
104-110: Protected fields for internal usage
Defining these fields as protected supports controlled extension in subclasses while preserving internal invariants.
110-110: Public uuid
Exposing a unique identifier can help track looker instances, especially in multi-instance scenarios.
171-171: Initialize asyncLabelsRenderingManager
Instantiating a dedicated manager streamlines label rendering tasks and aligns with the new typed approach.
612-623: Refactor refreshSample with consolidated options
Grouping the rendering parameters under anoptionsobject in the job request improves clarity, reduces argument length, and ensures consistent usage across the codebase.
benjaminpkane
force-pushed
the
looker-typing-cleanup
branch
from
2f12ce0 to
1a4bd93
Compare
What changes are proposed in this pull request?
Linting types in Looker for a bit more readability. No functional changes
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit