It'd be worth bring a brief description into the summary here.
Also cross references should be in the description or thread only, not in the title.

I updated the loop to an inline function. I believe this is a good compromise between code duplication and understanding behavior. Ignoring whitespace, renderers.py only gains seven lines, which is pretty minimal.

Are there any plans to merge this? It's just the thing I need to get the BrowsableAPI to work when posting multiple objects.

Thanks for the note - yes it's in the backlog, and we'd merge an acceptable pull request here.

Review: The pull request as it currently stands deals with two separate concerns. There's a refactoring, and there's also the re-try. It'd be easier to comprehensively review if these were treated as separate pull requests. Best thing to move this along would be for someone to take on the task of splitting this into two separately reviewable units, so that we can keep each step as absolutely minimal and thoroughly reviewed as possible.

Would splitting it into two separate commits in this pull request suffice? The two changes don't really make sense separately.

Any word on this?

No change on my original comment.

I'd want to see:

1. A refactoring PR that does not change the behavior, but is valid as it stands.
2. A behavioral PR on top of that.

Right now I don't have a lot of overhead for issue backlog - dealing with remains of 3.3/kickstarter functionality in last few days of funded time. Hopefully be able to get cracking with it again if we have a subsequent funding drive.

I do not understand why you want to see this as two PRs rather than two commits in one PR.
The refactor is meaningless without the behavior change. What does anybody gain from multiple Pull Requests for one issue?

Specifically, this is one logical change:

1. A commit which breaks the tests, providing an example of what is broken and a way to test that it is fixed.
2. A commit that splits off the behavior we want to modify to fix the broken test, but does not change functionality.
3. A commit to change the functionality in a way that fixes the test.

I can't make a PR for (1) by itself because that will break the tests by design.
Making a PR for (2) is asking you to review and merge a commit that appears to have no point, then starting over.
Making a PR for (3) requires that (2) have already been merged.

This is one change. You can take it or leave it. I am done jumping through hoops trying to make your software better for now.

I'm still waiting for resolution on this, and I'm not alone.

I've added a test that demonstrates the bug, I've made a non-functional change that helps with the fix (at your request), and I've fixed the bug. It's about as simple as a unit of work can get.

@asedeno Noted. We've got 114 open issues, and since the end of the kickstarter currently have no funded work on the project, so there's a backlog right now.

I understand there is a backlog. That would be more compelling if I were asking you to fix a bug, not handing you a fix for it myself. I put the work in for you, free of charge, and I will make any functional changes you might want to this PR, but I believe it is ready and has been for months.

Milestoning to move it up the queue.

Thank you!

So does a milestone mean anything for this PR, or is it only a can to keep kicking down the road?

Checks pass, there are no conflicts, this has been essentially ready for months.

This is very far from being a simple, obviously correct or easily reviewable change.

It's not immediately apparent to me whatexactly the behavior is before or after, and I've not any spare time to review in depth.

Havingtry...except TypeError reads completely opaquely, so even as the author I wouldn't understand what's happening there or why. If this was a simpler and more obvious fix, then yes it'd be in - as it stands, it's not terribly clear and hence, given the review bottleneck we've got at the moment it's stalled. Perhaps there's a simpler more obvious fix for this - again, not sure without some proper thinking about it.

Is the milestone meaningful? yes - it means this isn't absolutely at the bottom of the queue, and if and when we get paid time back it's likely to get resolved.

The try...except TypeError is there to catch
TypeError: 'ListSerializer' object is not iterable
If I could catch a more specific exception, I would. I can add more comments to the code if you like.

This is nothing more than:

1. try it with the serializer we have, like we do right now.
2. If we had a serializer and it fails with a TypeError, try again without the serializer like we would if we didn't have one.

In all the cases where DRF currently works, no TypeError is raised, so the try...except doesn't do anything. In the case where a TypeError is raised, DRF currently crashes and burns. This tries something else before crashing and burning, and the test I added shows that it does enough to make it succeed.

Again, this exception is raised after DRF has aperfectly valid response to the user's request, while rendering theuser-friendly browsable api and trying to pre-fill a form. Returning a response and a form that is not filled in is muchfriendlier than returning aTypeError when we have a perfectly good response to the user's request.

Noted. One comment for consideration. If the form rendering horribleness that we currently have can at least be nicely encapsulated then this could be a go-er.

Pulling the inner function out causes a lot more code churn, adds a lot of function parameters to replicate the enclosing scope, and I think actually makes this harder to reason about. This encapsulates just the piece ofget_rendered_html_form we want to retry so we can call it without a serializer.

This is one of the reasons python has scoping rules that allow for nested scoping. (Thank you, PEP 227.)

I don't have any huge problem with an inner scoped function.
I still have a problem with relying on outset scoped variables inside the inner scoped function, tho.
(i.e.has_serializer, others?)

The whole point of statically nested scoping (PEP 227) is to have access to the enclosing scope.

I'll refer you toviewsets.py where an innerview function makes use ofactions from the enclosing scope. Also, just about every decorator ever.

whole point
It keeps the function out of the global scope, which if it's not useful elsewhere is a nice bit of encapsulation.

I'll refer you to
Is a different case - we need to take that approach there because we're returning a function closure.

Those are side issues tho - In either case I still not convinced it's the right approach to keep it nested here as we're only increasing the complexity, without mitigating that by encapsulating any of it away. (Since we're in the same scope) Wrap it up more strictly and at least we're keeping the horribleness contained somewhat.

I think you are wrong. That said, I'm looking at a different approach that will maybe make you happier:

Collect a list of serializers to try with. If there is an existing serializer, that one will be first on the list. Next on the list is the serializer we would set up if there was not an existing serializer.

Then, loop over them in a try block and return the form using the first one that succeeds. The only question is, if they all fail, and there were multiple attempts, which exception would to like to see raised?

I've gone with the last one because that is the one I can raise with the right stack trace.

Is this implementation more to your liking? There's no inner function, and the loop is much more manageable.

Waking up this PR once again as I am still waiting for feedback or for it to be merged.

Okay, finally picked up as I now havefunded time on the project.
Fully reviewed now, and have an implementation based on this at#4181.
Thanks for the work towards this!