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

Fix type checks and typos #60

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Conversation

jaagut
Copy link
Contributor

@jaagut jaagut commented Mar 6, 2023

This PR fixes several type checker errors and typos in

  • ipm_image_node
  • ipm_library

@jaagut jaagut requested review from Flova and ijnek March 6, 2023 17:28
@jaagut jaagut self-assigned this Mar 6, 2023
Copy link
Contributor

@Flova Flova left a comment

Choose a reason for hiding this comment

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

Also is there any easy way of running the type checks in the CI?

@@ -23,7 +23,10 @@
from rclpy.duration import Duration
from rclpy.executors import MultiThreadedExecutor
from rclpy.node import Node
from sensor_msgs.msg import CameraInfo, Image, PointCloud2, PointField
from sensor_msgs.msg import CameraInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

What style did you use? Because I am pretty sure it is okay to group imports in the ros2 one as long as they are in the correct order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was isort with --profile google as you suggested.

I dont know whats right, but I like this more than the brackets () method😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I really hate it... it is so verbose ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

from geometry_msgs.msg import Transform
import numpy as np
from rclpy.duration import Duration
from rclpy.time import Time
Copy link
Contributor

Choose a reason for hiding this comment

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

What type check did you use? Was it static or during runtime? Because I am quite sure the time used here is the time message. And the rclpy time has a time source etc. and this is not compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Static.

The types from TF said its this one

Copy link
Contributor

Choose a reason for hiding this comment

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

But here we put it into a message and the message has this type.

@jaagut
Copy link
Contributor Author

jaagut commented Mar 6, 2023

Also is there any easy way of running the type checks in the CI?

Dont know yet, but we could commit workspace settings for IDEs, that turn on Type checking.

As Bitbots, we will discuss this on wednesday

@ijnek
Copy link
Member

ijnek commented Mar 10, 2023

Thanks for the improvements @jaagut! I have the same points as raised by @Flova, and the rest looks good to me.

@Flova
Copy link
Contributor

Flova commented Nov 29, 2023

Ping @jaagut

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.

3 participants