-
Notifications
You must be signed in to change notification settings - Fork 533
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
Recognize our review capacity is limited for non-trivial work #1417
Comments
I definitely understand that this is frustrating -- and I'm very sorry much of your hard work is not getting the attention that I agree it deserves. I've thought about this quite a bit, so here are some thoughts:
I don't feel like the right solution here is to review less -- I feel it as a responsibility to carefully review code going into chrono and I don't (yet) trust you to maintain it without me. I think it could help to carefully and explicitly prioritize what we are both motivated to work on and to limit ongoing work to fewer things at a time. It feels bad that I'm constraining your time/energy to improve chrono but I haven't been able to come up with a better solution. |
Hi! I was just passing by considering to contribute to chrono and wondering if I can help out here.
And I second @djc's comment about chrono being in a pretty good state. We're happily using 0.4 in uutils/coreutils and the 0.5 branch is looking better than ever! |
I definitely recognize that! Crossing off emails, but getting overwhelmed by the number of them that come in for a project and then parking them all for a while 😄.
I have said it before, but really mean it: I have a lot of respect for everything you do! You are a responsible maintainer and have taken on much more responsibility that I would want (for open-source work that is).
Completely agree. And I am happy with that.
You're reviews have definitely help me to improve my coding, and I appreciate them.
Sorry, can't agree there. The last time we discussed this you also suggested we work on shrinking the API and work towards 0.5 (including the fallibility and formatting work) instead of 0.4. Is there any middle ground we can find? Edit: Another option is suggested at some point is that we discuss some PRs on the desirability / risks / API changes only. |
Why can't you agree?
What freedom do you want? If you spend your time on the 0.5 branch without me reviewing I will feel obligated to review all of your changes at some point before it releases, which is unlikely to ever happen. If you prefer, you can fork chrono and start a separate project with a chrono-inspired API that conforms to your vision for what chrono 0.5 should be. But that project will no longer be chrono.
I feel a responsibility to review every line of code/comment changed in the core library. This is not because I like it (I definitely get more stress from maintaining chrono than I derive satisfaction from it) but because the previous maintainer entrusted me with this crate and it has lots of people depending on it. As such, I don't think this will change until/unless a maintainer comes along who I trust to apply a similar level of scrutinee. My obligation in maintaining chrono is towards chrono's users first, and towards contributors second. I'm sorry if that doesn't work for you (and some chrono users might get disappointed if you no longer want to work this way -- though I'm afraid only a tiny fraction of users pays attention) but I don't feel like I have many degrees of freedom here. |
Believe me, I only want to either help out with chrono or switch to another hobby. Introducing a fork or a third date and time library into the ecosystem is not for me. What I am looking for is not really freedom. Let's phrase this issue differently:
|
It will be driven to completion, but I can't give any guarantees how much time it takes. I am motivated to get this work done, but it will need to be done in small increments (#1410 is a good start). |
Same for me 😄. I'll split it into the smallest units that still make sense. |
This issue is not meant to complain, but to discuss a problem that in my opinion we have with our process.
Doing trivial changes is going great. Over the past year more than 150 PRs by me have been merged. Those usually only took me a couple of hours or less to write.
Doing non-trivial changes is very, very hard.
Example: The work to fix
DateTime
out-of-range panics.The initial PR that did 90% of the work was written in my spare time over ca. one a week (#1048). It was split in 8 different PRs (#1310, #1313, #1069, #1312, #1070, #1317, #1071 and #1333). Most landed over the course of half a year, with one still open.
Example: The work to convert the API to work in const context
This has just been completed! It took six PRs (#1043, #1080, #1205, #1286, #1337 and #1400). The total amount of work it was could have easily been done in two weeks, but it took 9 months.
Example: Fixing our offset parsing support
My work on this was finished around the end of May 2023. One part has been completed at the end of June (#1082), #1145 was merged at the end of august, part 2 (#1083) is still open, and part 3 is ready waiting on the merge of part 2 to become a PR.
Example: New formatting API
This was very difficult work (for me) that took ca. 3 weeks of spare time (#1196). So far none of it has seen a review, including simpler split out work in #1184, #1163 or #1335.
Example: Fixing panics on Windows
Local
Work on this was completed in ca one week in April 2023 (#1017, my third PR to chrono). That PR has been all over the place. It was extended to unify part of the Unix and Windows code, to handle parts that later became the '
DateTime
out-of-range panics'-work (#1048), further refactorings of the platform code (#1072), further refactorings of the Unix side, optimizations, and scaled back to just Windows.We could have long shipped it, as the core of the PR remained unchanged. Any improvements could have been later PRs.
Example: work on support for parsing ISO 8601 formats
Work on this was completed over a few days, and has since June lingered in #1143.
Example: work on adding a
CalendarDuration
The basic PR to add the type was opened in September (#1290). I have two branches to start doing meaningful operations on it. It is going to be a couple of weeks worth of work more to finish adding this type, spread over a whole bunch of PRs that have to depend on each other.
Example: adding a niche to
NaiveDate
Work started in #1207 in July. That lead to a refactoring of the
internals
module in #1212 in August. Part of the work was split of to another branch for a follow-up PR. The refactoring is now blocking other work in the module.Example: convert our API to return
Result
s.This is again going to be work that will require many PRs that build upon each other. Because of how much it would conflict with the work to make our methods const starting on it was delayed from June last year until now.
It can probably be finished in two months of work spread over ca. 15 PRs. But at the rate previous non-trivial work has gone it may well turn out to take more than a year, maybe even two.
The work to get a 0.5 release
If we want to make the API changes for 0.5 that have proven to be desirable in our issue tracker, that can very well be completed in less than a year of work for me, maybe even half a year. At the current rate it is going to take 4 times longer or more.
The process introduces issues
I like the extra quality reviews bring. However the current process introduces more issues than it catches. If a PR has to rebase 10+ times that is likely to introduce issues. After a couple of months the details are paged out for me. If a lot of code around a PR changes, some of the changes may be subtle enough to slip through while I would have caught them while writing the code. Review is not catching those.
The process is draining
Curating open PRs by rebasing them to keep them in a merge-able state is easily taking ca. ⅓rd of my time. This can be spend better. To friendly keep asking for reviews while recognizing that all we do is volunteer work is not fun. Seeing all the work that took a lot of effort go to waste by being unmerged, or merged with an inordinate amount of effort from my side is simply draining for the motivation.
Not everything can be reduced to trivial PRs
Some bug fixes or improvements just require a 1000+ lines to change. It can be split in small(ish), self-contained commits. That may take me double the effort, but I am more than willing to do that. But I can only meaningfully test something is fixed with the full set of changes.
Of course a 12-commit PR can be split into 6 PRs that build upon each other. We have done so for some of the work mentioned above. I am willing to keep making a 'master-PR`, provided the split our PRs are going to make progress in a reasonable time frame.
Work starts blocking each other
On average I have 25+ open PRs, and 25 other branches that can't even be opened as PR because they depend on them. I have to keep the state they are in in my mind. How do they depend on each other?
"Didn't I fix this issue before? Why am I touching the same code again? O yes, it is in one of the unmerged branches from a couple of months ago."
At some point it just becomes difficult to find parts of the codebase I can work on that don't cause a massive merge conflict with already open PRs.
How to fix?
I'll leave that to @djc. Hopefully we can relax the review process, as I can't just demand more time from you.
I love working on chrono. But the process to do non-trivial work has already drained my motivation once.
The text was updated successfully, but these errors were encountered: