Skip to content

Origin/feature/omh to ros dev - #2

Open
moperezs wants to merge 25 commits into
mainfrom
origin/feature/omhToRosDev
Open

Origin/feature/omh to ros dev#2
moperezs wants to merge 25 commits into
mainfrom
origin/feature/omhToRosDev

Conversation

@moperezs

Copy link
Copy Markdown
Collaborator

No description provided.

@moperezs moperezs left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great work!! It already looks very gueet.
Some general comments:

  • Please add an empty line at the end of each file
  • Check that all functions have types in inputs and expected outputs
  • Did you format the files (e.g. with blackformatter or mypy)? Some lines seem quite long here in git.
  • I moved the omh_streamer to the root directory, because it just streams data, which i would separate from converting data.
  • From my point of view, conversions should work online and offline. The user should choose whether they implement the library in either case. But I see here you separated online and offline, is there a reason for it?

Comment thread .gitignore

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I would remove this .gitignore from the repo and let everyone have their custom configs if needed.

Comment thread examples/notebooks/generated_ros_msgs/Bcg.msg
Comment thread examples/notebooks/generated_ros_msgs/EcgInfo.msg
Comment thread examples/notebooks/generated_ros_msgs/BcgInfo.msg
Comment thread offline_converter/omh_to_ros_schema/converters/__init__.py Outdated
Comment thread offline_converter/omh_to_ros_schema/utils/schema_loader.py Outdated
Comment thread offline_converter/ros_standardization/converters/__init__.py Outdated
Comment thread offline_converter/ros_standardization/converters/device_info_converter.py Outdated
Comment thread online_converter/omh_ros_converter/test/test_v2.py Outdated
Comment thread online_converter/omh_ros_converter/test/test_v2.py Outdated
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