-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add logger, tests and refactor #62
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
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, left some comment with a cosmetic suggestion.
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.
In the project, somewhere, we use List
and Tuple
in the function/method declaration, but sometimes we use list
and tuple
. Could we please unify this?
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.
This is difference between python3.8 and python3.10.
Are there some functionalities (other than these type annotations) used from python3.10? Otherwise we can still keep minimum version to 3.8 and change the type hints to List, Tuple, ....
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.
Just a note; python 3.8 reached its EOL this month, do we want to keep supporting it? IMHO we should migrate datadreamer
and luxonis-ml
to 3.10 sooner rather than later.
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.
@kozlov721 I'd keep support for 3.8 support for now. Or are there any drawbacks?
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
Test Results 6 files 6 suites 44m 33s ⏱️ Results for commit 05b13be. ♻️ This comment has been updated with latest results. |
…datadreamer into refactor/tests-refactor
…datadreamer into refactor/tests-refactor
This PR includes:
For larger test coverage more powerful runners (like BuildJet) should be used.