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

Ros2 whisper #279

Open
wants to merge 11 commits into
base: ros2
Choose a base branch
from
Open

Ros2 whisper #279

wants to merge 11 commits into from

Conversation

maayan25
Copy link
Collaborator

No description provided.

Copy link
Member

@jws-1 jws-1 left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work! I haven't been able to test, as I'm having some issues with the ROS2 container on my laptop. Most of the changes requested are to do with deleting stuff that I don't think we necessarily need, but please correct me if I'm wrong!

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this

This package is maintained by:

- [Maayan Armony](mailto:[email protected])
- [Paul Makles](mailto:[email protected]) (ROS1)
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
- [Paul Makles](mailto:[email protected]) (ROS1)

This package is maintained by:

- [Maayan Armony](mailto:[email protected])
- [Paul Makles](mailto:[email protected]) (ROS1)
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
- [Paul Makles](mailto:[email protected]) (ROS1)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed, unless something has changed in ROS2

Copy link
Member

Choose a reason for hiding this comment

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

Same as requirements.in

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this whole directory and it's contents, as it's related to the old version of our transcription service

Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is needed!

Copy link
Member

Choose a reason for hiding this comment

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

Is this something ROS2 specific & do we need 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.

2 participants