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 --yaml-extend option to allow modifying rosdoc2.yaml #151

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

rkent
Copy link
Contributor

@rkent rkent commented Oct 30, 2024

This PR allows the person running rosdoc2 to specify a yaml file that will override documentation options that are set in a package.

I see two primary context that this might be used.

First, in a demo build such as I currently show at https://rosdoc2.rosdabbler.com/, it allows me to demonstrate to package authors what their rosdoc2 output would look like if they did certain recommended changes to their configuration. I don't believe this is a particularly controversial use, and it will be helpful to anyone working with package authors to improve their usage of rosdoc2.

The second possible context is to use a centrally-curated extension file to either fix or modify the configuration of packages in the standard runs of rosdoc2. I don't know if this will be a good idea or not, but whether we do this or not is a decision for a different time. But this could be used, for example, to disable running doxygen in packages where the output is both useless and very time consuming.

There was some refactoring to allow this to be done that is some of the complexity of this patch.

The extension file is typically divided into sections, that may have special options (mostly to be added later), but also according to their usage. You can see the extension file I am currently using here which gives you an idea on how this would be used.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Overall I think that this is a good idea. It's great for trying broad tweaks such as testing an option in development.

One question I have is can we avoid the significant chunk of custom merge code and reuse an existing yaml merge/overlay implementation? Such as converting the yaml to a dict and calling update.

There's pretty compact ways to do a deep update https://stackoverflow.com/questions/3232943/update-value-of-a-nested-dictionary-of-varying-depth

And the only_if_missing seems to be able to be achieved by inverting the order of the updates.

I think part of my challenge understanding the complexity of the merge is that the extension format is not documented. So maybe adding that would help clarify that. Documenting the formatting and optoins like 'only_if_missing`.

Another approach to that would be to have simply two different command line arguments one is extend and one extend-if-missing to do that switch and not embed that into the yaml format itself.

Signed-off-by: R. Kent James <kent@caspia.com>
@rkent
Copy link
Contributor Author

rkent commented Nov 14, 2024

I redid this with the feedback from the review, but I did not do all that was requested.

  1. "can we avoid the significant chunk of custom merge code and reuse an existing yaml merge/overlay implementation"

I've tried various more standard merge options, but there are a lot of subtleties to how this is done. See https://stackoverflow.com/questions/7204805/deep-merge-dictionaries-of-dictionaries-in-python Whenever I have tried to use standard implementations of deep merges, these subtleties have forced me to tweak them (I face a very similar issue in https://github.com/rkent/distroclone where the merger is into rosdistro instead of rosdoc2.yaml). Using a tweaked "standard" deep merge makes the code obscure (if sometimes shorter) and I'm never happy with the result. It seems more straightforward to be explicit.

I did though try to simplify this. I got rid of the "options" completely, I am not actually currently using that, and I only added the 'only_if_missing" choice as an example, and that seemed to be the source of some of the complexity. That option would make sense if we were to try to use this in the standard rosdoc2 builds, so that package author's changes were implemented, but trying to do it now is premature. I did leave in the "packages" key though so that an option could be added in the future.

I also documented the format as requested.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks that looks a lot cleaner and with the extra documentation of the structure it's easy to understand how this will work.

@tfoote tfoote merged commit 8c0e785 into ros-infrastructure:main Nov 15, 2024
5 checks passed
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