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

Performance improvements, new tests, more typing annotations and miscellaneous fixes #967

Open
wants to merge 66 commits into
base: main
Choose a base branch
from

Conversation

Rouslan
Copy link

@Rouslan Rouslan commented Jan 24, 2024

This pull request, along with the improvement made in the upcoming version 7.3 of sphinx, should fix the issue of Breathe being too slow.

The changes in this pull request are massive and I understand that the maintainers have very little time to spare, so I don't expect this will be merged right away, and I don't mind putting in more work if it's needed to get this accepted.

@michaeljones
Copy link
Collaborator

Just wanted to say this is a deeply impressive amount of work and thank you for taking the time to do it. I hope you have enjoyed the process in some way.

This represents a substantial change to the project. Most likely for the better but still substantial. I have not been active on this project for some time though I am still one of the maintainers as such. I think this PR might renew interest in the project for some users and certainly resolve issues for them. The concern is that it might also require some testing and on going maintenance.

I think the options are:

  • Merge the work here and maintain it ourselves
  • Merge the work here and make @Rouslan a collaborator on the project and hope that you continue to have the energy to maintain this work going forward.
  • Recommend that @Rouslan forks this project in some fashion and maintains a separate instance of it, likely under another name, that users can migrate to over time.

The first option is unappealing without funding from the user base. I'm not sure which is better from the other two options. The third option is the simplest from the perspective of the maintainers of Breathe, I imagine, though I don't speak for my other two maintainers.

I do not wish to appear ungrateful. I can see the amount of work and effort you've put in and I'm grateful for the efforts to communicate along the way. Thanks again.

@Rouslan
Copy link
Author

Rouslan commented Jan 31, 2024

I only have respect for people that maintain free software long term. I don't expect anyone to take time away from whatever they were doing just because I took it upon myself to volunteer some code, regardless of effort.

Anyway, I'm afraid I don't wish to be responsible for Breathe in any major way, but I'll support the parts I've changed, at least for a year. I'll fix any bugs in my code, answer any questions about the code and even make changes if asked.

I can continue to maintain this fork as long as it is only to verify stability/correctness before eventually merging (no sense in changing the name in this case). The other maintainers are free make other changes to the fork; I just won't be taking an active role in that (with the exception of one feature I wanted to add, for which I'll make a separate branch).

@vermeeren
Copy link
Collaborator

@michaeljones @Rouslan I feel like the second option proposed is good.

Merge the work here and make @Rouslan a collaborator on the project and hope that you continue to have the energy to maintain this work going forward.

I'm very grateful for any help with Breathe honestly. Considering it has been on a best-effort basis for many years already large changes like these can just be shipped without extensive testing in my opinion. If the CI tests pass (may require some other open PR changes for newer Sphinx etc) and it works fine or a few large test projects in the wild I consider it tested enough.

At the end of the day we just don't have the resources for full regression tests and the like. Moving the project forward in a healthy way is more important in my opinion, even if it may break some edge cases once in a while for certain users.

Cheers

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.

None yet

4 participants