Closed plowsof closed 2 years ago
Thanks for taking care of this @plowsof, very needed. Soon i'll be able to dedicate some time to reviewing PRs, but in the meantime i noticed there is a stream of PRs changing the same file:
Merging these PRs would result in 15 commits for the same file, it's a significant bloat for git that can be easily avoided. Would be better instead to have one single commit and a single WIP PR.
@erciccione we already planned to merge them to a single PR with separate commits, he is currently working on it. Keeping them together in a single commit is not necessary, there is no such thing as git bloat (on a technical level, it's basically personal preference).
@selsta even if git bloat doesn't exist (and my understanding is that it does, unless something has changed in the last years. I struggle to think that's the case though, even only because each pr also creates a merge commit. So merging all the PRs would have resulted in 15 commits + 15 merge commits. Definitely unnecessary and real bloat), opening multiple PR for the same file is something we have never did before in this repository (but i remember luigi asking to group PRs in other repositories too), so i don't see the reason to deviate from this approach.
For reference, every time there was to update the dev guide i always opened a single draft PR where i kept track of the calls that needed to be added and when i had all of them or most of them, i opened a single pr. IIRC this was specifically requested from luigi, but i might be remembering wrong. In any case that's what we have been doing until now.
That said, luigi is the maintainer so he has to be ok with this approach in case.
edit:
we already planned to merge them to a single PR with separate commits
I haven't found any conversation about this in #monero-site, so maybe this discussion happened privately? I suggest to discuss this sort of things in the dedicated public room, so that long term contributors and maintainers can weight in before novel approaches are introduced.
I never advocated for having multiple PRs for a single file, I was for multiple commits in a single PR. No extra work for luigi, no merge conflicts and it allows us to retain authorship information.
This has been a learning experience for me, i used the multiple PR's as a way to get them individually reviewed / approved. Later on i cherry-picked each and handled the annoying conflicts one-by-one (to save luigi doing this)
Some days ago, I asked selsta to review some of the PR's that where ready and he made a passing comment that there will be merge issues with all these PR's editing the same 2 files (i was unaware of the extent to which this would be a problem) - i then began taking the approved PR's and placing them into a single PR / closing my others.
This all happened within the last 24 hours and it is my fault for not discussing my plans / intentions publicly, so i apologise for any confusions caused. Moving forward i will make sure to discuss my plans publicly, and keep changes to the same file(s) inside 1 PR.
The new PR with multiple commits has been merged for luigi so there are no conflicts and indeed , it retains credit where credit is due (when i have taken other peoples answers). I have not heard his opinion on that PR yet.
✅ Deploy Preview for barolo-time-757cf9 ready!
Built without sensitive environment variables
Toggle QR Code...
Use your smartphone camera to open QR code link.
To edit notification comments on pull requests, go to your Netlify site settings.