-
Notifications
You must be signed in to change notification settings - Fork 38
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
Document and improve wavemap configuration #28
Conversation
This seems cleaner than applying conditional schemas based on custom properties (e.g. $schema: wavemap), which doesn't work well with CLion and might also break things in other user's pipelines.
/prepare-release minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that asking for a solution description is needed or beneficial, as that might just result in unusable ideas. I would however have a section that asks what that feature is supposed to be useful for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote this template from scratch, hopefully it's better now.
Measurement inputs | ||
****************** | ||
These settings control the measurement input handlers. | ||
They are nested in the top level config under ``inputs``. Note that ``inputs`` takes a YAML list, and one input handler will be created for each item in the list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is YAML list correct given all this is JSON?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that people will primarily use wavemap with ROS, the settings would be specified in ROS config files (YAML).
The only place where we currently use JSON is when defining the schemas for wavemap configs, which are defined using the jsonschema standard. These schemas are used for linting (pre-commit) and can be used to simplify config editing (in CLion), but don't affect wavemap's runtime.
Based on your comment I briefly considered changing "YAML list" to just "list", but I think it's good to keep the former s.t. users can quickly find the syntax they need for YAML lists on Google. Let me know if you agree/disagree :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation looks good, and I like the entire "make it hard to mess things up with typos and unit errors".
Thank you @LionelOtt for reviewing, and astumpf for contributing! |
New features:
livox_ros_driver2
dependency is not found (contributed by astumpf).livox
but the driver is not installed, they will be prompted to install it.Notes for the review:
docs/pages
directory. The rendered new docs can be explored locally by downloading this archive, unzipping theartifact.tar
it contains, and opening theindex.html
file in a browser.std::optional<Config>
return values, but introducing exceptions is a breaking change I would rather properly look into later.