Skip to content
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

[GSoC2024] add return types to fix typescript warnings #7689

Merged
merged 14 commits into from
Apr 18, 2024

Conversation

tahamukhtar20
Copy link
Contributor

@tahamukhtar20 tahamukhtar20 commented Mar 27, 2024

Fixes: #7598

Motivation and context

There are some minor typescript errors in the files, this pull request fixes those.

How has this been tested?

Ran the code after changing, and it was functional like before, this didn't need any additional test.

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@tahamukhtar20
Copy link
Contributor Author

I think this PR is not ready yet, there are a few changes which I just realized need to be added, I'll add those and ping you accordingly, thanks.

Copy link

codecov bot commented Mar 27, 2024

Codecov Report

Merging #7689 (8f352a8) into develop (02889ee) will increase coverage by 0.01%.
The diff coverage is 75.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7689      +/-   ##
===========================================
+ Coverage    83.36%   83.37%   +0.01%     
===========================================
  Files          373      373              
  Lines        39722    39722              
  Branches      3747     3747              
===========================================
+ Hits         33113    33118       +5     
+ Misses        6609     6604       -5     
Components Coverage Δ
cvat-ui 79.23% <75.00%> (+0.03%) ⬆️
cvat-server 87.21% <ø> (-0.01%) ⬇️

@@ -0,0 +1,4 @@
### Fixed

- Fixed typescript linting errors in ts files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix has no impact on the user, so I don't think it needs a changelog entry.

@nmanovic nmanovic requested review from bsekachev and removed request for nmanovic March 28, 2024 14:51
@bsekachev
Copy link
Member

Hello, do you have any plans regarding the pull request?

@tahamukhtar20
Copy link
Contributor Author

Yes @bsekachev , I left a comment above under your review, please let me know. Thanks.

@bsekachev
Copy link
Member

Yes, one week ago you mentioned "tomorrow". That is why I am asking :)

@tahamukhtar20
Copy link
Contributor Author

Yes, one week ago you mentioned "tomorrow". That is why I am asking :)

Sorry about that, @bsekachev I wanted to ask something about your comment under the Promise review, you said we should use something more concrete here instead of any, but that is Lambda manager, and we are very uncertain what will be returned here, hence doesn't any make sense here?

@bsekachev
Copy link
Member

bsekachev commented Apr 10, 2024

I believe we are certain enough. Otherwise how would it be possible to work with this API?
As far as I remember run() returns a string, representing rq identificator to fetch automatic annotation statuses later.

call() is more complex, but, more or less, you may find it out from this file cvat-ui/src/components/annotation-page/standard-workspace/controls-side-bar/tools-control.tsx.

Perhaps the output is different for trackers, detectors and interactors.
So, it should be like Promise<TrackerResults | DetectorResults | InteractorResults>, and define these three types additionally.

@tahamukhtar20
Copy link
Contributor Author

Hi @bsekachev, I've added the required types and currently added the interfaces on top of the Lambda Manager file, please let me know if that is fine or if there is some other place more appropriate for the interfaces.
Additionally, I have updated the concerned code related to the lambda.call function, but I had to make some type assertions(e.g. number[] and number[][] in the tools-control.tsx file) to make it work with the existing code without making too many changes. Please let me know if this is the correct implementation for this.
Thank you.

cvat-core/src/lambda-manager.ts Outdated Show resolved Hide resolved
cvat-core/src/lambda-manager.ts Outdated Show resolved Hide resolved
cvat-core/src/lambda-manager.ts Outdated Show resolved Hide resolved
cvat-core/src/lambda-manager.ts Outdated Show resolved Hide resolved
@bsekachev
Copy link
Member

Please, avoid changes in tools-control.tsx, I will work with types inconsistency on my own, I see there are some issues.

tahamukhtar20 and others added 4 commits April 17, 2024 14:54
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
@tahamukhtar20
Copy link
Contributor Author

@bsekachev Updated

@bsekachev bsekachev merged commit 4ed6d04 into cvat-ai:develop Apr 18, 2024
15 checks passed
@bsekachev
Copy link
Member

Just FYI: #7785

@tahamukhtar20
Copy link
Contributor Author

I found some more typescript warnings because of missing return types, maybe I can create a follow-up pull request for this if you allow it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linting warnings in some typescript files
3 participants