search

ros-infrastructure / rosdoc2

Command-line tool for generating documentation for ROS 2 packages.
Apache License 2.0
39 stars 9 forks source link

Make sure both 'settings' and 'builders' are in the YAML. #5

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

That is, don't fall back to empty defaults for them. This is important in case users provide their own YAML configuration, but have a typo in them (like 'biulders' instead of 'builders'). With the default of an empty dict, the builders would just be skipped completely, and could lead to hard-to-debug issues. At least with this PR it would be louder.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

@wjwwood @tfoote This seems fairly harmless to me, but an opinion on whether this is the right way to go is appreciated.

tfoote commented 3 years ago

The biggest question is at some point do we see the default builder(s) changing at some point down the line. If so having everyone hardcoded with the current defaults will make that transition much harder. On the flip side, they will keep working if the new builders need a migration. I agree that we should have an error the extra key rejection could cover that case, in which case I'd probably lean towards suggesting that approach instead of adding more required things that might need migrating in the future. We should probably also make sure that the "default builders (x y z) selected due to no config element in the yaml" message be printed promenantly for visibility/discoverability.

clalancette commented 3 years ago

Just to be clear, this PR doesn't change anything about the default behavior, just the behavior when the user specifies a config file. Some examples:

Scenario Before this PR after this PR
No rosdoc2.yaml specified Use default builders (doxygen and sphinx) Use default builders (doxygen and sphinx)
rosdoc2.yaml specified with 'settings' and 'builders' Use builders specified by user Use builders specified by user
rosdoc2.yaml specified with no 'builders' and no 'settings' Use default settings, don't run any builders Error
rosdoc2.yaml specified with 'builders' but no 'settings' Use builders specified by user, and default for settings Error
rosdoc2.yaml specified with 'settings' but no 'builders' Don't run any builders Error

Because of that, I think it is still easy to change the default set of builders in the future. The only thing this PR makes is so that once you specify something in rosdoc2.yaml, you have to specify everything. We could think about making this a little less explicit, but I feel like that is easy enough to do later. So I'm going to go ahead and merge this for now, thanks for the conversation.