feat: generic body parser

[Phillip9587]

As an alternative to #561. All previous authors have been mentioned with Co-authored-by instead of a rebase.

I made some additional cleanups that I mentioned in #561.

[wesleytodd]

LGTM. We should close the other and see if we can get the folks who approved there to give this one last look before merging.

[Phillip9587]

@wesleytodd Thanks. Could you please request a review from them, as I don't have the necessary permissions?

[wesleytodd]

Ah, interesting. We should get you those perms. I would do it now but I have to head afk and I think it may require us adding you as a collaborator with write persm. I will leave this tab open for later and follow back up.

[bjohansebas]

I think it was better to squash the commits via the dropdown instead of creating a new PR.

[ctcpip]

sorry but I see mistakes in this PR now and it's not possible to determine where they were introduced. (e.g. a bad rebase conflict, or a refactoring or something else)

[Phillip9587]

Hi @ctcpip,

I wanted to express some frustration about the current situation with this PR and #561. Both have received approvals from other team members. I understand you might have concerns, but it's really hard to address them when there's no clear explanation of why you're holding things up.

The lack of clarity is making it difficult to move forward, especially since others on the team have already approved the changes. Could you please share more details on what’s holding things back or what changes you'd like to see? It would help a lot to get everyone aligned and make progress.

Looking forward to hearing your thoughts.

[ctcpip]

Hi @Phillip9587, I appreciate the effort you've made and I understand how delays can be frustrating.

I want to clarify that I’ve already provided feedback on #561, which was:

I would like to see the rebase commits cleanly applied so that git history makes sense. there are two or three commits after the rebase that should not be necessary

The rebase was not done cleanly, leading to multiple 'fix rebase' commits. It's important that the rebase is done cleanly. So the rebase would need to be started over, and extra scrutiny when resolving so this was not repeated.

I also would much prefer that your refactoring commit have been done as a separate PR so we could evaluate those particular changes made in isolation.

I don't mind if the rebase work was all squashed, but I'd like to see that first, and be able to review that for correctness, with any refactoring PRs to follow on separately.

but wait...

Probably more importantly: the reason the original rebase PR was sitting around to begin with was because repo captain @jonchurch was opposed to landing this change in general due to some concerns; and we never came to a resolution on that. I don't want to misrepresent his position, so I will defer to him to elaborate.

[ctcpip]

Hi @Phillip9587 -- I went ahead and did clean squash and rebase here: #574

This does not include any of your further refactoring; it's the rebase only, and squashed to avoid having to do the extensive rebase yet again later.

[wesleytodd]

Sorry, what was the goal here? Before I go try to read and compare more PRs I want to make sure I know what I missed in this. Can you be more descriptive about what mistakes, maybe comment on those lines specifically?

[Phillip9587]

Hi @ctcpip,

I want to clarify that I’ve already provided feedback on #561, which was:

I would like to see the rebase commits cleanly applied so that git history makes sense. there are two or three commits after the rebase that should not be necessary

I created this PR as a cleaned up alternative to #561, with all commits, including my refactorings from feedback on #561, shashed together and mentioning the authors from #544, on which this work was based.

I also would much prefer that your refactoring commit have been done as a separate PR so we could evaluate those particular changes made in isolation.

This PR and #561 were never meant to be just a rebase of #544. There were some risks and advancements that I discovered and pointed out in #561.

Probably more importantly: the reason the original rebase PR was sitting around to begin with was because repo captain @jonchurch was opposed to landing this change in general due to some concerns; and we never came to a resolution on that. I don't want to misrepresent his position, so I will defer to him to elaborate.

That's the reason why I applied my own work on top of that. I just wanted to honor the foundational work by Shawn Dellysse and you. Given that #561 received three approvals and this PR has been approved by @wesleytodd, I believe we are well-positioned to finalize and merge this significant improvement for our users. I’d greatly appreciate it if you could review my changes and provide feedback, which I’ll address promptly.

Thanks

[Phillip9587]

Hi @wesleytodd, I hope my last comment clarified my perspective. I would greatly appreciate it if you could consider reopening this PR.

[wesleytodd]

So one good lesson from this is never close a PR and reopen a new one with a rebase, instead force push to the existing PR branch. That keeps the conversation consolidated and coherent. With this new one we now have conversation spread across like 4 PRs. This goes as well for the "here is a clean rebase" PR @ctcpip, as that was not necessary because we can push to PR branches to fix them if we need to.

Ideally we could coach @Phillip9587 through the correct steps to achieve a clean rebase (but I didn't see that happen here, sorry if it did happen in a previous PR thread). This approach should avoid both the clear miscommunications which are happening here and the frustration hopefully.

So to recap, here are the open threads of this conversation:

1. There was old objection to the approach from @jonchurch which is present in some old thread not linked in this new one (maybe @ctcpip can find the link to that as the person who mentioned it)
2. There is a current objection by @ctcpip to this PR which I dont think I fully understand. There are two parts I will address below

The rebase was not done cleanly, leading to multiple 'fix rebase' commits.

This PR has only one commit, so I dont think I follow what the feedback is here. Is this for the previous PRs? If so, maybe we can make that more clear in the future? Again, ideally we dont do this "close a pr and supersede it with a new one", -f is your friend. If it is something else I am missing, please lets be as clear as we can.

I also would much prefer that your refactoring commit have been done as a separate PR so we could evaluate those particular changes made in isolation.

This is good feedback I (mostly) agree with. We can always sqaush it again when we are ready to merge, but in this case it might have made the work easier to follow. That said, it is harder to do stacked PRs from a fork than necessary so personally I would also have been alright with a single PR with multiple commits and a comment in the PR description to help reviewers.

---

My preferred next step would be the following:

1. Close yet another rebase! generic body parser #574 and this feat: generic body parser #572 (this one) and any others I may have missed in the process
2. Cleanly rebase and force push to the original PR (if we need to help coach through how to do that we can)
3. Re-apply the work from this PR to the original one as a second commit on top of the rebase
4. And only then address @jonchurch's concerns if they are still present

Sound like a plan?

[ctcpip]

This goes as well for the "here is a clean rebase" PR @ctcpip, as that was not necessary because we can push to PR branches to fix them if we need to.

This would have been destructive. If I force pushed over Phillip's PR, I would erase the additional work he did, and if I force pushed over the older one, I would erase the (often valuable) history of the individual commits. If I had made a mistake somewhere along the way in the newest rebase, then we wouldn't have the original. This is the entire reason why I created a new rebased PR and closed the original

So, please do not overwrite branch contents at this point.

That said, this is a bit of an uncommon situation, with such a long-lived PR sitting out there. Branches and PRs are cheap, and it is unfortunate to have fractured discussion.

Unfortunately @Phillip9587 has not been in the loop on the discussions we've had about this feature in general and Jon's hesitation to land it. It doesn't appear there is any paper trail of this -- it's only been in discussions in meetings and possibly in slack.

As it stands, #574 is currently the only branch/PR that contains the original generic parser impl in isolation without conflicts.

So I suggest we use that one unless someone again wants to try to do clean rebase with all the individual commits.

A further refactoring is fine, but let's please do that as a separate PR.

BUT still, we never got through Jon's block and we need to resolve that first before deciding to land anything.

[wesleytodd]

#524 (comment)

Collecting the relevant comments spread out to clarify our next steps.

[ctcpip]

@Phillip9587 here is the PR for the refactoring on top of the original changes:

#575