ros-infrastructure / rosdoc2

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

Change the YAML builder configuration arguments. #8

Closed clalancette closed 3 years ago

clalancette commented 3 years ago

Make the builders a list, where the key for each one is the type of builder and the values is a dictionary of required and optional keys.

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

This may be the most important one that we agree on, since it fundamentally changes the configuration file that users provide (and thus it will be hard to change later). Opinions welcome here.

clalancette commented 3 years ago

Excellent points, thanks William.

I like your idea about the list of dictionaries, though I would change it slightly:

builders: [
  - {
    type: 'doxygen',
    name: '{package_name} Public C/C++ API'
    output_dir: 'generated/doxygen'
  }
  - {
    type: 'sphinx',
    name: '{package_name}',
    output_dir: ''
  }
]

I'm going to go ahead and code that up.

wjwwood commented 3 years ago

Ok, sounds good.

Also I should say that the reason the key being a builder was concerning for me was that I have been toying with the idea that the landing page should just be another Sphinx builder invocation. But I'm not firm on that yet.

clalancette commented 3 years ago

I like your idea about the list of dictionaries, though I would change it slightly:

Actually, after looking at it, I decided that your idea was best, so I went with that.

Also I should say that the reason the key being a builder was concerning for me was that I have been toying with the idea that the landing page should just be another Sphinx builder invocation. But I'm not firm on that yet.

Your idea makes perfect sense, users may want to run things more than once for some reason. I've now tested that you can specify multiple of the same 'type' of builder with the new code now, and it works. Please take another look when you get a chance.

clalancette commented 3 years ago

Thanks for the review, going ahead and merging this one.