Opened 3 years ago

Closed 2 years ago

#5395 enhancement closed fixed (fixed)

resource that can render a twisted.web.template

Reported by: glyph Owned by: dreid
Priority: normal Milestone:
Component: web Keywords:
Cc: jknight Branch: branches/elementresource-5395
(diff, github, buildbot, log)
Author: MostAwesomeDude Launchpad Bug:

Description

Questions like this one on stack overflow shouldn't need to be asked; there should be a Resource subclass within Twisted which knows how to render your template for you.

See also #4983 for addressing this problem with documentation.

Change History (31)

comment:1 Changed 3 years ago by DefaultCC Plugin

  • Cc jknight added

comment:2 Changed 2 years ago by MostAwesomeDude

  • Author set to MostAwesomeDude
  • Branch set to branches/elementresource-5395

(In [33837]) Branching to 'elementresource-5395'

comment:3 Changed 2 years ago by MostAwesomeDude

(In [33839]) Add ElementResource, test, and topfile.

Time for reviews. Yay?

Refs #5395

comment:4 Changed 2 years ago by MostAwesomeDude

  • Keywords review added

Reviews please. This is super-simple, so I'm sure that there's obvious things I'm missing, but it can't be that difficult of a task.

comment:5 Changed 2 years ago by dreid

  • Keywords review removed
  • Owner set to MostAwesomeDude
  1. Oh man, return 1 is terrible. If we're going to do that (and since it was glyph's idea I assume we are) you should expand on the comment why you're doing it and how it works.
  2. For callbacks that ignore their result lambda _: seems to be the most common pattern in twisted. Followed by lambda ign:
  3. Lines 547-549 of test_template.py should be indented one more level.
  4. You committed these changes to the wrong branch.
  5. I'm not entirely sure this should be render_GET vs just render. At which point it should probably just implement IResource itself vs subclassing Resource.

comment:6 Changed 2 years ago by MostAwesomeDude

(In [33844]) t.w.template: Cleanup names, comments.

Refs #5395

comment:7 Changed 2 years ago by MostAwesomeDude

  • Keywords review added
  • Owner MostAwesomeDude deleted
  1. Commented on "return 1" and put it at the top with a better name.
  2. Changed that.
  3. Changed that too.
  4. Derp. :T Fixed it.
  5. Nope, as dicsussed in person, doing this kind of rendering doesn't really make sense for non-GET.

Thanks for the review.

comment:8 Changed 2 years ago by dreid

  • Keywords review removed
  • Owner set to MostAwesomeDude

Ok, this looks better, as per discussion with glyph it sounds like this shouldn't actually be a resource it should be a function that can you can call as:

    def render_GET(self, request):
        return renderElement(request, MyElement(…))

And maybe in a separate ticket there should be a resource which has the same semantics as this resource and calls renderElement for you.

Also:

  1. the deferred from flatten should have an errback.
    1. it should call log.err
    2. it should check request.site.displayTracebacks
      1. it should render a and write out a twisted.web.util.FailureElement
    3. it should call request.finish

comment:9 Changed 2 years ago by MostAwesomeDude

  • Keywords review added
  • Owner MostAwesomeDude deleted

Okay. Let's go for it again. dreid has recused himself, so somebody else will need to review.

comment:10 Changed 2 years ago by wsanchez

  • Owner set to wsanchez
  • Status changed from new to assigned

comment:11 Changed 2 years ago by wsanchez

  • Keywords easy review removed
  • Owner changed from wsanchez to MostAwesomeDude
  • Status changed from assigned to new

In template.py, renderElement(), we should be rendering some sort of error message (but no traceback, or other leakage) if not request.site.displayTracebacks, rather than rendering nothing at all.

Over in test_simpleFailure(), there is code that is checking that we render nothing:

    self.assertEqual("".join(self.request.written), "")

But that would prevent renderElement() from passing if it did render a useful error message.

Nit: Glyph notes that the spacing conventions for code (three lines between classes, two between methods) aren't being followed here.

comment:12 Changed 2 years ago by dreid

(In [33847]) Implement error output when displayTracebacks is False.
Improve docstrings.
Correct spacing.

Refs #5395

comment:13 Changed 2 years ago by dreid

  • Keywords review added
  • Owner MostAwesomeDude deleted

I went ahead and dealt with the review from wsanchez. Mostly I added docstrings, fixed spacing, added an error for when we don't want to display the traceback. I also refactored the tests slightly to avoid invoking _render.

comment:14 Changed 2 years ago by wsanchez

  • Keywords review removed
  • Owner set to dreid

Yes, this makes the sense.

The only thing I might reconsider is the use of <h1> for the error message. Since the error could happen in a nested element, an <h1> may not be appropriate. I think a weenier web weenie than I might prefer a generic <div class="RenderingError"> or some such thingo that could be styled. But that could be addressed in another ticket from someone how has run into this and has more thoughtfulness about that the right way to do that is.

comment:15 Changed 2 years ago by dreid

(In [33853]) Render a better error. Now it is big and red on an explicit white background.

Refs #5395

comment:16 Changed 2 years ago by dreid

(In [33855]) Fix missing end quote on style attribute.

Refs #5395

comment:17 Changed 2 years ago by dreid

  • Keywords review added
  • Owner dreid deleted

comment:18 Changed 2 years ago by dreid

  • Keywords review removed
  • Owner set to dreid
[13:11:13] <exarkun> dreid: were you asleep during the `@d.addCallback´ conversation?
[13:13:05] <exarkun> dreid: It's easy to fix.  Please do.

comment:19 Changed 2 years ago by dreid

(In [33858]) Address exarkun's review about @d.addCallback being bad.

Refs #5395

comment:20 Changed 2 years ago by dreid

  • Keywords review added
  • Owner dreid deleted

comment:21 Changed 2 years ago by wsanchez

  • Owner set to wsanchez
  • Status changed from new to assigned

comment:22 Changed 2 years ago by wsanchez

  • Owner changed from wsanchez to dreid
  • Status changed from assigned to new

+1

comment:23 Changed 2 years ago by wsanchez

  • Keywords review removed

comment:24 Changed 2 years ago by dreid

(In [33859]) Assert that only one error is flushed.

Refs #5395

comment:25 Changed 2 years ago by dreid

  • Keywords review added
  • Owner dreid deleted

exarkun had some informal review which I addressed.

comment:26 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:27 Changed 2 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to dreid
  • Status changed from assigned to new
  1. twisted/web/template.py
    1. Please file a ticket for fixing NOT_DONE_YET. There are other ways to avoid the circularity without relying on accidental (!!) integer identity semantics. Link to the ticket from that comment.
    2. Always pass a message argument to log.err. renderElement is calling it with only a failure argument.
    3. renderElement needs to be in __all__
  2. twisted/web/test/test_template.py
    1. TestElement needs a class docstring
    2. TestFailureElement too
    3. The docstrings for the test methods on TestRenderElement should not start with "Test that ...".
    4. Some of the indentation is funny. It should all look like the indentation in test_simpleFailure, for the assertion self.request.written.
    5. assertEqual is not the right check to use with NOT_DONE_YET. It should be assertIdentical, because the implementation uses is (in one place, == in another, ha ha).
    6. Please remove the blank line at the beginning of some of the test methods.

Thanks very much for your work on this, and to the efforts of all the previous reviewers. I think after these changes are made the branch will be in good shape, and you should merge it if you get a good build on buildbot.

comment:28 Changed 2 years ago by dreid

(In [33887]) Fix indentation, docstrings, NOT_DONE_YET assertions.

Also reference the ticket for fixing the circular NOT_DONE_YET imports.

Refs #5395

comment:29 Changed 2 years ago by dreid

(In [33888]) Deal with circular dependency between util and template using the
fully qualified name of FailureElement.

Refs #5395

comment:30 Changed 2 years ago by dreid

Builds succeeded except for intermittent and usual windows bugs. http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/elementresource-5395

comment:31 Changed 2 years ago by dreid

  • Resolution set to fixed
  • Status changed from new to closed

(In [33889]) Merge elementresource-5395: Add renderElement, for rendering Elements.

Authors: MostAwesomeDude, dreid
Reviewer: wsanchez, exarkun
Fixes: #5395

Add renderElement, a function for rendering Elements from resources. It
returns NOT_DONE_YET, writes the template incrementally to the request, and
handles errors during rendering. It will display a traceback if the Site
is configured to do so.

Note: See TracTickets for help on using tickets.