-
Notifications
You must be signed in to change notification settings - Fork 49
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
Revert "revive support for the multiproject monkeypatch (#139)" #146
Conversation
This reverts commit bd5fcd7.
@clalancette do you have a small example of a configuration which would get you this bug? I can't quite figure out what configuration would make the insert go haywire. If you already have a if not any(":project:" in spec for spec in ret):
ret.insert(0, ":project: " + configs._the_app.config.breathe_default_project) instead of always inserting the |
Gah! I have company this week but still take a look this weekend. Yeah can you please link the build setup that causes this so I have something additional to test against? I'm kind of surprised that this happens given how it works and demanding |
So I spent some time yesterday prototyping what the "right" fix should be, but my company decided to stick around so I'm not going to be able to do much about this or #147 until saturday. AFAICT the underlying problem is @florianhumblot if I can't get a fix in soon, would you be OK with a backup plan of a dedicated monkeypatch hack branch that just gets archived in the future but rebased? They spent a lot of time helping get exhale back on its feet (as well as a lot of improvements for breathe) so I want to undo breaking them 😶 |
@svenevs I don't mind per se, I can even stay on the current version for now, but it would be a lot nicer to be able to work this in correctly, since that way we can all stay on the main branch :P |
Prevents multiple `:project:` tags from being added to a breathe directive. Fixes the issue that PR svenevs#146 tries to prevent.
@svenevs I was able to reproduce @clalancette's issue when using rosdoc2 and building the documentation for rcl_lifecycle while on exhale's current master commit. Steps were:
This would produce a bunch of errors relating to the project tag being added:
Inserting if not any(":project:" in spec for spec in ret): before inserting Current code, which breaks rosdoc2: # the monkeypatch re-configures breathe_default_project each time which was
# foolishly relied on elsewhere and undoing that blunder requires undoing
# all of the shenanigans that is configs.py...
ret.insert(0, ":project: " + configs._the_app.config.breathe_default_project)
return ret Updated code, which does not break rosdoc2: # the monkeypatch re-configures breathe_default_project each time which was
# foolishly relied on elsewhere and undoing that blunder requires undoing
# all of the shenanigans that is configs.py...
if not any(":project:" in spec for spec in ret):
ret.insert(0, ":project: " + configs._the_app.config.breathe_default_project)
return ret I've added the "fix" to this PR: #148 @clalancette could you confirm that that addresses your concerns about the |
Thanks for the help @florianhumblot ❤️ Pinged the other contributors on that project in #148 but I think it's good to go, and a hack on top of a hack is totally valid in this scenario 😭 Either way, I don't want to revert that commit, if things don't work then I'll do the actual fix that is needed but that may break more for them which will somewhat turn into "lets find the right way to wrap exhale" which is more effort than any of us want to put in right now. |
Prevents multiple `:project:` tags from being added to a breathe directive. Fixes the issue that PR svenevs#146 tries to prevent.
Prevents multiple `:project:` tags from being added to a breathe directive. Relates: #146
This reverts commit bd5fcd7.
The monkey patching is causing a lot of problems in our usage of exhale. In particular, every single include directive is getting put into the output files multiple times, like this:
This PR somewhat controversially reverts the monkey-patching, getting us back to a state where things mostly work (I still have one other WARNING to chase down, but I'll do that separately). @svenevs Thoughts on this?