Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix ros1 video_activity_recognition.py #322

Merged
merged 10 commits into from
Sep 29, 2022
Merged

Fix ros1 video_activity_recognition.py #322

merged 10 commits into from
Sep 29, 2022

Conversation

LukasHedegaard
Copy link
Collaborator

This PR updates video_activity_recognition.py as described in #281 and addresses #302

@tsampazk tsampazk added the test sources Run style checks label Sep 28, 2022
Copy link
Collaborator

@stefaniapedrazzi stefaniapedrazzi left a comment

Choose a reason for hiding this comment

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

Thank you!

Changes seems fine to me.
However, it seems that you are using a different max line width than the 128 characters specified in the OpenDR coding styles and this produced many unrelated changes:
https://github.com/opendr-eu/opendr/wiki/Coding-Style/#cs101-dont-exceed-128-character-for-line-of-code

Also note that there would also be the README.md file to update accordingly:
https://github.com/opendr-eu/opendr/blob/fdd730c0dcb0218b300e8f29e6a5468c8f8c1fdb/projects/opendr_ws/src/perception/README.md

These changes to the README.md have not been merged to develop yet so it would not be optimal to update the README.md in this branch.
So you could add your changes directly in the #316 branch or create a new PR to merge your changes into #316.

@LukasHedegaard
Copy link
Collaborator Author

Thank you!

Changes seems fine to me. However, it seems that you are using a different max line width than the 128 characters specified in the OpenDR coding styles and this produced many unrelated changes: https://github.com/opendr-eu/opendr/wiki/Coding-Style/#cs101-dont-exceed-128-character-for-line-of-code

Also note that there would also be the README.md file to update accordingly: https://github.com/opendr-eu/opendr/blob/fdd730c0dcb0218b300e8f29e6a5468c8f8c1fdb/projects/opendr_ws/src/perception/README.md

These changes to the README.md have not been merged to develop yet so it would not be optimal to update the README.md in this branch. So you could add your changes directly in the #316 branch or create a new PR to merge your changes into #316.

Thank you for the comments, @stefaniapedrazzi.
The line-breaks with end-commas follow the "black" linter style, which does adhere to OpenDR coding style as well. There is no minimum line-break requirement as far as I can see. As to the end-comma, it is increasingly common, as it lets you swap function arguments quickly without risk of forgetting a comma.

As to the README, I'll make sure to update the #316 branch accordingly

Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

Thanks @LukasHedegaard for the node fixes! Changes look good, apart from some minor comments. Unfortunately, i was not able to run the node due to some errors.

@tsampazk tsampazk self-requested a review September 29, 2022 10:02
tsampazk
tsampazk previously approved these changes Sep 29, 2022
Copy link
Collaborator

@tsampazk tsampazk left a comment

Choose a reason for hiding this comment

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

Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test sources Run style checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants