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

imshowパッケージをc++からpythonに変更 #26

Closed
wants to merge 2 commits into from

Conversation

23-yoshikawa
Copy link
Contributor

No description provided.

@23-yoshikawa 23-yoshikawa requested a review from H1rono June 21, 2024 11:43
Copy link
Member

@H1rono H1rono left a comment

Choose a reason for hiding this comment

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

camera_readerにも変更を入れちゃってますが、このPRでは関係ないものなので戻してください

Copy link
Member

Choose a reason for hiding this comment

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

これ消さないで欲しいです
提供するnodeとexecutableは変わらないので、記述を変える必要もない

Copy link
Member

Choose a reason for hiding this comment

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

フォーマットが気になるので、可能なら整形しておきたい
参考: PEP8

Copy link
Member

Choose a reason for hiding this comment

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

具体的には、

  • importclassの間は空行2行
  • class内の関数定義間は空行1行
  • 関数呼び出しの引数リストで,の後に空白1マスを入れる

app/imshow/package.xml Show resolved Hide resolved
Comment on lines +10 to +13
<test_depend>ament_copyright</test_depend>
<test_depend>ament_flake8</test_depend>
<test_depend>ament_pep257</test_depend>
<test_depend>python3-pytest</test_depend>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<test_depend>ament_copyright</test_depend>
<test_depend>ament_flake8</test_depend>
<test_depend>ament_pep257</test_depend>
<test_depend>python3-pytest</test_depend>
<exec_depend>rclpy</exec_depend>
<exec_depend>sensor_msgs</exec_depend>
<exec_depend>cv_bridge</exec_depend>

Copy link
Member

Choose a reason for hiding this comment

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

この変更いらないのでは?

Comment on lines +13 to +14
description='Image display for ROS2',
license='MIT license',
Copy link
Member

@H1rono H1rono Jun 23, 2024

Choose a reason for hiding this comment

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

package.xmlと揃えるようにお願いします

Copy link
Member

Choose a reason for hiding this comment

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

これだけだとlaunchファイルがビルド結果に含まれないので、そこの記述を足してください
↓記述例

data_files=[
('share/ament_index/resource_index/packages',
['resource/' + package_name]),
('share/' + package_name, ['package.xml']),
(os.path.join('share', package_name, 'launch'), glob(os.path.join('launch', '*')))
],

Copy link
Member

Choose a reason for hiding this comment

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

これを足すとimportが足りなくなるのでそれも忘れずに(これも記述例参照)

Copy link
Member

Choose a reason for hiding this comment

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

テストしないので消してください

Copy link
Member

Choose a reason for hiding this comment

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

テストしないので消

Copy link
Member

Choose a reason for hiding this comment

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

テストしないので

@H1rono H1rono closed this Jun 23, 2024
@H1rono H1rono deleted the imshow_python branch June 23, 2024 12:56
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.

None yet

2 participants