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

Add source entry for ros2_openai_server (jazzy) #42014

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

AndyZe
Copy link
Contributor

@AndyZe AndyZe commented Jul 9, 2024

@github-actions github-actions bot added the jazzy Issue/PR is for the ROS 2 Jazzy distribution label Jul 9, 2024
@marcoag
Copy link
Contributor

marcoag commented Jul 10, 2024

OpenAI is an organization, the name of this package seems a bit misleading on what the actual service is or what is calling.

@marcoag marcoag requested review from marcoag and Yadunund July 10, 2024 07:53
@marcoag marcoag self-assigned this Jul 10, 2024
@marcoag marcoag added the changes requested Maintainers have asked for changes to the pull request label Jul 10, 2024
@AndyZe
Copy link
Contributor Author

AndyZe commented Jul 10, 2024

I'd be fine with chatgpt_server if that floats your boat. I'm aware that I should drop ros2_ from the front

Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

Package name details
$ find . -name "package.xml" -exec grep --color=auto -e "<name>" "{}" ";"
<name>ros2_openai_server</name>
License details
$ find . -name "package.xml" -exec grep --color=auto -e "<license>" "{}" "+"
<license>Apache-2.0</license>

Echoing @marcoag sentiments I too think the naming is very generic and does not pass REP 144. It would be good to properly suffix the package, eg. robosoft_ai_openai_server. (ros2 can be dropped imo but not a strict requirement).

jazzy/distribution.yaml Outdated Show resolved Hide resolved
@AndyZe
Copy link
Contributor Author

AndyZe commented Jul 11, 2024

OK, I can live with robosoft_openai_server or robosoft_openai

@marcoag marcoag requested a review from Yadunund July 11, 2024 03:47
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating!

@marcoag marcoag merged commit e6fdae2 into ros:master Jul 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Maintainers have asked for changes to the pull request jazzy Issue/PR is for the ROS 2 Jazzy distribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants