cvat-core api fixes & refactoring #8016
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes across several files in the CVAT codebase focus on refactoring methods to enhance clarity, flexibility, and maintainability. Key modifications include updating methods to accept object parameters instead of individual parameters, improving parameter handling, and refining error handling. These updates streamline the code, making it more readable and easier to manage. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
cvat-core/src/annotations-collection.ts (1)
[!TIP]
Codebase VerificationThe verification process reveals that not all calls to the
clearmethod incvat-core/src/annotations.ts,cvat-core/src/annotations-history.ts, andcvat-core/src/session-implementation.tspass the appropriate flags object. Specifically:
- In
cvat-core/src/annotations.ts, the call tocleardoes pass theflagsobject.- In
cvat-core/src/annotations-history.ts, the call tocleardoes not pass any flags.- In
cvat-core/src/session-implementation.ts, the calls tocleardo not pass any flags.These findings indicate that the refactoring to accept a
flagsobject has not been consistently applied across the codebase.
cvat-core/src/annotations-history.ts: The constructor callsthis.clear()without any flags.cvat-core/src/session-implementation.ts: Theclearmethod is called without any flags in theclearActionsImplementationandTask.prototype.actions.clear.implementationfunctions.Action Required:
- Update the calls to
clearincvat-core/src/annotations-history.tsandcvat-core/src/session-implementation.tsto pass the appropriateflagsobject.Locations to Update:
cvat-core/src/annotations-history.ts: Line 6cvat-core/src/session-implementation.ts: Lines 5 and 14Analysis chain
Line range hint
769-803: Refactoring theclearmethod to accept aflagsobject enhances flexibility and clarity. Ensure all calls to this method have been updated to pass the appropriate flags object.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `clear` method pass the appropriate flags object. # Test: Search for the function usage. Expect: Only calls with the new flags object. rg --type typescript $'\\.clear\\('Length of output: 69
Script:
#!/bin/bash # Description: Verify all calls to `clear` method pass the appropriate flags object. # Test: Search for the function usage. Expect: Only calls with the new flags object. rg --type ts '\.clear\('Length of output: 1576
Script:
#!/bin/bash # Description: Extract context around calls to `clear` method in relevant files to verify if they pass the appropriate flags object. # Extract context around calls in cvat-core/src/annotations.ts rg --context 5 --type ts '\.clear\(' cvat-core/src/annotations.ts # Extract context around calls in cvat-core/src/annotations-history.ts rg --context 5 --type ts '\.clear\(' cvat-core/src/annotations-history.ts # Extract context around calls in cvat-core/src/session-implementation.ts rg --context 5 --type ts '\.clear\(' cvat-core/src/session-implementation.tsLength of output: 1824
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- cvat-core/src/annotations-collection.ts (5 hunks)
- cvat-core/src/annotations.ts (2 hunks)
- cvat-core/src/session-implementation.ts (2 hunks)
- cvat-core/src/session.ts (6 hunks)
- cvat-core/tests/api/annotations.cjs (14 hunks)
- cvat-core/tests/api/frames.cjs (4 hunks)
- cvat-ui/src/actions/annotation-actions.ts (2 hunks)
- cvat-ui/src/actions/import-actions.ts (1 hunks)
- cvat-ui/src/containers/annotation-page/top-bar/annotation-menu.tsx (1 hunks)
- tests/cypress/support/commands.js (1 hunks)
Additional context used
GitHub Check: Linter
cvat-core/src/session.ts
[failure] 29-29:
'Camelized' is defined but never used
Additional comments not posted (32)
cvat-ui/src/containers/annotation-page/top-bar/annotation-menu.tsx (1)
53-54: Parameter names inremoveAnnotationshave been updated for clarity.Please ensure that any related documentation is updated to reflect these changes.
cvat-core/tests/api/frames.cjs (1)
50-50: Updatedclearmethod calls to use object parameters.Please verify that all tests pass with the new method signature.
Also applies to: 60-60, 70-70, 80-80
cvat-ui/src/actions/import-actions.ts (1)
115-115: Updatedclearmethod call to use an object parameter inimportDatasetAsync.Please ensure that the function behaves as expected with the new parameter format.
cvat-core/src/annotations.ts (1)
116-130: RefactoredclearAnnotationsto use aflagsobject parameter, enhancing flexibility and clarity.Consider adding unit tests to cover the new functionality and ensure that existing tests are updated to reflect these changes.
cvat-core/src/session-implementation.ts (2)
57-572: RefactoredJobclass implementations are well-structured and maintain consistency.The use of
Object.definePropertyfor method implementations ensures encapsulation and control over property behaviors, which is a good practice in complex systems. However, ensure thorough testing, especially for asynchronous operations and error handling paths.
Line range hint
579-937: RefactoredTaskclass implementations follow a consistent pattern and improve maintainability.Similar to the
Jobclass, theTaskclass usesObject.definePropertyeffectively. It's crucial to verify that all new implementations interact correctly with existing functionalities and that side effects are handled appropriately.cvat-core/tests/api/annotations.cjs (18)
250-250: Refactor to use object parameter forclearmethod aligns with API changes.
263-263: Refactor to use object parameter forclearmethod aligns with API changes.
280-280: Refactor to use object parameter forclearmethod aligns with API changes.
310-310: Refactor to use object parameter forclearmethod aligns with API changes.
331-331: Refactor to use object parameter forclearmethod aligns with API changes.
346-346: Refactor to use object parameter forclearmethod aligns with API changes.
366-366: Refactor to use object parameter forclearmethod aligns with API changes.
381-381: Refactor to use object parameter forclearmethod aligns with API changes.
412-412: Refactor to use object parameter forclearmethod aligns with API changes.
772-772: Refactor to use object parameter forclearmethod aligns with API changes.
820-820: Refactor to use object parameter forclearmethod aligns with API changes.
833-833: Refactor to use object parameter forclearmethod aligns with API changes.
841-841: Refactor to use object parameter forclearmethod aligns with API changes.
849-849: Refactor to use object parameter forclearmethod aligns with API changes.
857-857: Refactor to use object parameter forclearmethod aligns with API changes.
865-865: Refactor to use object parameter forclearmethod aligns with API changes.
879-879: Refactor to use object parameter forclearmethod aligns with API changes.
958-958: Refactor to use object parameter forclearmethod aligns with API changes.cvat-core/src/session.ts (6)
8-8: Import ofChunkQualityfromcvat-dataadded.This import is necessary for the updated method signatures that use
ChunkQualityas a parameter type.
28-28: Import ofIssueadded.This import is used in the updated method signatures and implementations related to issue handling in the
Jobclass.
30-30: Import ofObjectStateadded.This import is crucial for the type annotations in methods that deal with object states, enhancing type safety and clarity.
60-62: Refactoredclearmethod to accept aflagsobject.This change aligns with the PR's objective to enhance parameter handling and maintainability by using object parameters instead of multiple individual parameters.
314-365: Updated method signatures in theSessionclass to include specific parameter and return types.These changes improve type safety and clarity, making the API more robust and easier to use correctly.
Line range hint
662-682: Enhanced error handling and specific return types in theJobclass methods.These updates are part of the extensive refactoring effort to improve the reliability and maintainability of the code.
cvat-ui/src/actions/annotation-actions.ts (1)
378-383: RefactoredremoveAnnotationsAsyncto use object parameters for clarity and maintainability.tests/cypress/support/commands.js (1)
272-272: The update to pass an object{ reload: true }instead of a booleantruealigns with the overall refactoring strategy to improve parameter handling across the codebase.
bsekachev
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8016 +/- ##
===========================================
- Coverage 83.55% 83.48% -0.08%
===========================================
Files 384 384
Lines 40350 40415 +65
Branches 3771 3772 +1
===========================================
+ Hits 33716 33740 +24
- Misses 6634 6675 +41
|
bsekachev
force-pushed
the
bs/refactoring
branch
from
fd3cb21
to
ea95925
Compare
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Refactor
flagsobject parameter for better clarity and flexibility.Tests
Chores