Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Markupsafe friendly #1

Merged
merged 0 commits into from Apr 3, 2012

Conversation

Projects
None yet
2 participants

bukzor commented Apr 2, 2012

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.

@bukzor bukzor and 1 other commented on an outdated diff Apr 2, 2012

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 Apr 2, 2012

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

@eevee

eevee Apr 3, 2012

Owner

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

@bukzor

bukzor Apr 3, 2012

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

eevee Apr 3, 2012

Owner

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 Apr 3, 2012

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

@eevee eevee and 1 other commented on an outdated diff Apr 3, 2012

cheetah/Template.py
# 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

eevee Apr 3, 2012

Owner

no need for else after a return

@bukzor

bukzor Apr 3, 2012

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

eevee Apr 3, 2012

Owner

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 Apr 3, 2012

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

@eevee eevee commented on the diff Apr 3, 2012

cheetah/Tests/Filters.py
- # 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

eevee Apr 3, 2012

Owner

you don't think this is worth a test?

@bukzor

bukzor Apr 3, 2012

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

eevee Apr 3, 2012

Owner

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 Apr 3, 2012

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

eevee Apr 3, 2012

Owner

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

@bukzor

bukzor Apr 3, 2012

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.

@eevee eevee and 1 other commented on an outdated diff Apr 3, 2012

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

eevee Apr 3, 2012

Owner

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 Apr 3, 2012

Yes, you're correct. Reject this chunk.

@eevee eevee pushed a commit that referenced this pull request Apr 3, 2012

@bukzor bukzor after discussion with eevee #1 f365190

@eevee eevee merged commit f365190 into eevee:markupsafe-friendly Apr 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment