Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Markupsafe friendly #1

Merged
merged 0 commits into from

2 participants

@bukzor

This is a bit simpler and (for me) easy to follow.

I've also made cheetah functions return None in the transactional case, which should prevent some accidental errors.

cheetah/Compiler.py
@@ -1096,7 +1096,12 @@ def _addAutoCleanupCode(self):
self.addChunk('')
def addStop(self, expr=None):
- self.addChunk('return _dummyTrans and trans.response().getvalue() or ""')
+ if self.setting('autoAssignDummyTransactionToSelf'):
+ empty = None
@bukzor
bukzor added a note

Actually, this should probably be unequivocally 'return None' in the transactional case.

@eevee Owner
eevee added a note

Yeah, no need to distinguish between autoAssignEtc and a transaction by some other means, methinks.

@bukzor
bukzor added a note

That's little vauge. What you're saying seems more broad than what I was thinking:

if self.setting('autoAssignDummyTransactionToSelf'):
    self.addChunk('return None')
else:
    self.addChunk('return _dummyTrans and trans.response().getvalue() or ""')
@eevee Owner
eevee added a note

i mean why not just:

if _dummyTrans:
    return trans.response().getvalue()
else:
    return None

if you're using transactions at all, regardless of why or how, None is technically a correct response

@bukzor
bukzor added a note

Feel free to do that, but it's a stronger implementation than I had proposed (or tested).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cheetah/Template.py
((6 lines not shown))
# Template functions using transactions always return an empty
# string
return buffer
-
- # ...but this one didn't, which means either the function is a
- # plain Python function, or it's a template function that used
- # #return.
- # In the latter case, check that it didn't /both/ use #return /and/
- # try to write to the buffer. Without $capture but with a
- # transaction, such a function would normally write out /both/ its
- # contents /and/ its return value, and we're not going to do that.
- if buffer.strip():
- warnings.warn("Ignoring buffer contents due to use of #return in $capture(%r)" % function)
+ else:
@eevee Owner
eevee added a note

no need for else after a return

@bukzor
bukzor added a note

There's no need for it, but logically, it's an else-case, so I use the else-syntax.
It's just the same to the interpreter, but makes the control flow more visual.

@eevee Owner
eevee added a note

it backfires here, though: the return retval at the bottom looks, at a glance, like it sometimes happens after the if and sometimes happens after the else.

@bukzor
bukzor added a note

I did the indenting wrong :(
It originally lined up with the if buffer.strip: and that should have been preserved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@eevee eevee commented on the diff
cheetah/Tests/Filters.py
((11 lines not shown))
- # This will fail because $foo is substituted and filtered inside
- # the #call block, the function is run, and then the return value
- # (still containing $foo) is filtered again
- output = self.render("""
- #def print_foo: $foo
-
- #call $call_noop
- [$print_foo()]
- #end call
- """)
- except UniqueError:
- pass
- else:
- assert False, "UniqueFilter should have raised UniqueError"
-
- def test_fixed_call(self):
@eevee Owner
eevee added a note

you don't think this is worth a test?

@bukzor
bukzor added a note

I'm still testing this, but in a more specific way. The inline diff makes it hard to see...
Below, I assert that expected = '<2> [<1>bar</1>] </2>' which shows double-filtering, but more specifically and concretely.

@eevee Owner
eevee added a note

the point isn't just the double-filtering, though; it's also the unfiltering step we need for using #call with markupsafe.

the real goal of all this is to only have one layer of filter-wrapping at the very end.

@bukzor
bukzor added a note

Sorry my confusion: i thought you were referring to test_naive_call, which I've kept, but yes, I've removed test_fixed call.
The only thing that I could see that it tested was the fact that 'UniqueFilter.unfilter' works, which is not a feature of cheetah or something that needs to be maintained.

Is it testing more than that?

@eevee Owner
eevee added a note

well, it verifies that the thing we're trying to make work actually works :)

@bukzor
bukzor added a note

What it's really testing (beyond test_naive_call) is that you can also call functions which do str.replace, rather than just the identity function.
Pretty sure it's not useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cheetah/Tests/SyntaxAndOutput.py
@@ -2992,13 +2991,13 @@ def showwarning(message, category, filename, lineno, file=None, line=None):
"$buf $buf",
'output output'
)
+ except UserWarning:
@eevee Owner
eevee added a note

by just making the warning fatal, you never verify that the behavior is actually correct. that's why i did the showwarning nonsense.

@bukzor
bukzor added a note

Yes, you're correct. Reject this chunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@eevee eevee referenced this pull request from a commit
@bukzor bukzor after discussion with eevee #1 f365190
@eevee eevee merged commit f365190 into eevee:markupsafe-friendly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 0 additions and 0 deletions.
Something went wrong with that request. Please try again.