Performance improvements, new tests, more typing annotations and miscellaneous fixes #967
Conversation
…e into memberdef-in-groups
|
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:
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. |
|
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). |
|
@michaeljones @Rouslan I feel like the second option proposed is good.
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 |
|
Hi @Rouslan, Is there a sensible way of breaking this PR up into chunks? I'd like to get it merged, but for git-history reasons if anything else, it would be good to have some form of meaningful units of change. Best, |
|
I'm not sure I understand what you mean when you say "for git-history reasons" because, as I understand, merging the pull-request would add all 66 commits to the history, but regardless, breaking this up would be more trouble than it's worth. I started out by swapping the parser with an incompatible one, breaking a lot of things. The annotations were added gradually as I dug through the code, trying to figure out how it worked. As I adapted the code to the new parser interface, I refactored a lot of it. I started adding tests for my own sake as I gradually restored the original functionality. Almost all the changes are interdependent. Anyway, if you give me a couple of days, I'll merge the latest changes into my fork and work out the conflicts. I'll have to change the build system back to Setuptools because the new parser is generated from a schema (not an XML schema, I made my own simplified format) as part of a build step, which Flit doesn't allow. |
I think that's basically what @AA-Turner is trying to address. Ideally, all commits should be "bissectable", i.e. there should be no known regression between each commit and should a downstream user realize things break, they should be able to track what commit introduced the change, which is hard to do when there's lot of "non-atomic"/intertwined commits. |
|
@kartben covered most of the rationale, but to cover one point:
I would squash-merge which preserves a linear history -- each PR equates to one commit in master. A |
This is so the local "breathe" folder will have _parser.py
make "block" attribute optional for "htmlonly" tag Co-authored-by: Benjamin Cabé <[email protected]>
|
I finished the merge. I understand now what the purpose for breaking up the pull request would be. It's certainly possible, but it would take a while. I would have go through all my commits, one by one, and either selectively undo certain changes, or conversely, create a new branch and selectively apply certain changes. I would need to go through all the commits multiple times and reapply the changes for the subsequent pull requests. By the time I'm done, there would probably be new changes in the original repository, and translating new changes from the old code into the new code is also not fun. Either way, the first pull request would still be the biggest because the parser's data types permeate so much of the code. There was even code in the original parser that had to be moved outside the parser because it was specific to a much later step in Breathe's operation. I'm not enthusiastic about doing that. I would have to be convinced that smaller pull requests are worth the effort. Also, in case the tone of this text comes off as negative: I wish to clarify that I didn't mind being asked. |
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.