Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make rendering encoding configurable #44

Merged
merged 8 commits into from
Apr 25, 2019
Merged

Make rendering encoding configurable #44

merged 8 commits into from
Apr 25, 2019

Conversation

dataflake
Copy link
Member

@dataflake dataflake commented Apr 22, 2019

This replaces branch encoding-as-optional-argument and is based on current master, I broke that branch in a botched rebase operation.

This will start fixing zopefoundation/Zope#271

From Malthe's original PR #23:
This adds an optional keyword argument encoding to the publicly exposed rendering functions and classes.

See #20 for previous discussion.
Closes #20.
Closes #22.
Closes #23.

@dataflake dataflake added the bug label Apr 22, 2019
@dataflake dataflake added this to the Release 3.0 milestone Apr 22, 2019
@dataflake dataflake requested a review from icemac April 22, 2019 01:26
@dataflake dataflake self-assigned this Apr 22, 2019
@dataflake dataflake added this to In progress in Zope 4 final release via automation Apr 22, 2019
@icemac
Copy link
Member

icemac commented Apr 24, 2019

Hm, the tests are only green on Python 2 and there are some Flake 8 errors. @dataflake Are you planning to look into them?

@dataflake
Copy link
Member Author

This is work in progress and you know it's done when I ask for a review.

@dataflake
Copy link
Member Author

I just realized I had already sent a review request, sorry, that was premature.

@dataflake dataflake removed the request for review from icemac April 24, 2019 11:20
@dataflake dataflake requested a review from icemac April 25, 2019 03:25
Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

# This might be a TaintedString object
untaintmethod = getattr(t, '__untaint__', None)
if untaintmethod is not None:
# Quote it
t = untaintmethod()
skip_html_quote = 1

if not isinstance(t, basestring):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this work with all the unicode and basestring names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, we were defining them at the beginning of the module.

Copy link
Member Author

@dataflake dataflake Apr 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's many places where code wants to decide "is this a string" as opposed to, say, a callable. The work that's been done to make all that Python 3 compatible was really haphazard, inconsistent and incomplete. I've spent hours stepping through the debugger on this, and I am sure there will be more edge cases found in the future.

Zope 4 final release automation moved this from In progress to Reviewer approved Apr 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants