Getting feedback on WIP patches

I’m wondering where would be best to get feedback on partially complete patches? My thoughts are that initial ideas seem to be discussed on the ticket itself, but for getting an initial approach checked that it might be done in a Draft PR on Github?

I’m specifically thinking about a PR that is partially done but could use a sanity check before continuing down a certain path.

I think a draft PR would be fine, as long as it mentions the associated ticket - you could also post here and ask for feedback, and if it’s in one of our specialty areas we can jump on it.

It’s certainly not a perfect system, I’ll grant you. I also suspect with a draft PR, you might get one of the Django Fellows dropping by to offer advice.

Thanks, I’m still trying to figure out exactly what mechanisms work best for this to annoy the fewest people. For example, the contribution guide mentions that PRs should be a single commit, so does this mean that with draft PRs I’d need to keep force-with-lease pushing to the same branch? What about when someone has already reviewed it (I’m not sure if force-pushing erases comments on Github or anything like that)?

The PR is here and does point back to the ticket (here). It’s basically looking to make builtin decorators be able to handle both sync and async views without the need for additional @sync_to_async and @async_to_sync around them.

I’ve got what I think is a workable solution up, based on the discussion in the ticket, although there’s a lot of repetition so I’m trying to see if there’s a nice way of wrapping the main process_response function (which is the only thing that’s different in all of the functions). I’d be super keen for any pointers or obvious issues as it’s my first real foray into async things.

My personal approach is that it’s fine to have a draft PR have multiple commits as you’re working on it, and then just squash it down to a single commit when you’re ready for a “final” review.

As for the draft PR you linked - I’ll try to get some eyes on it this week, but a lot is going on for me this week so no promises!

No worries, I’ve got it up as a full PR now, albeit with a couple of weirdnesses / questions that I’ve detailed in the PR description. It’s also been changed to Ready for checkin so I’m hoping someone might get around to casting an eye on it at some point even if you’re busy :slightly_smiling_face:

Hey @LomaxOnTheRun — Welcome aboard! :sailboat:

More or less, what Andrew said. In the end it’ll be a single commit and so on — the Patch review checklist can help make sure you’ve ticked all the boxes — but to begin whatever lets you get it there.

We’re currently hovering around 30 PRs that are waiting for review, so it may be a week or two, but we’ll get to you!

I didn’t look in depth yet but, on this one, we will definitely ask for Andrew’s opinion, since it’ll touch on the design of the user-facing API of writing async views in Django, and that needs to be just right.

From the PR:

All documented view decorators are compatible with both synchronous and asynchronous views…

That sounds awesome! :grinning:

Thanks! I think I’ve hit everything on the checklist, so it’s good to sit there until someone has the time to look at it :slightly_smiling_face: it’ll also give me chance to catch up on some of the other things I’ve been putting off while getting this done :sweat_smile: